tilemap: UMD module build does not have a requirejs export

Currently the UMD module that is built is not really a complete UMD module. it will work without issue in the browser, but I see no “require” at all that would make it compatible with “CommonJS” and so it’s possibly not loaded properly in Electron.

https://www.davidbcalhoun.com/2014/what-is-amd-commonjs-and-umd/

(function (root, factory) {
    if (typeof define === 'function' && define.amd) {
        // AMD
        define(['jquery'], factory);
    } else if (typeof exports === 'object') {
        // Node, CommonJS-like
        module.exports = factory(require('jquery')); 
    } else {
        // Browser globals (root is window)
        root.returnExports = factory(root.jQuery);
    }
}(this, function ($) {
    //    methods
    function myFunc(){};

    //    exposed public method
    return myFunc;
}));

This is needed for pixi-tilemap to work in gdevelop https://github.com/4ian/GDevelop/pull/1901

About this issue

  • Original URL
  • State: open
  • Created 4 years ago
  • Comments: 26 (15 by maintainers)

Most upvoted comments

All done, please try again.

UMD link is correct. Require and ESM builds are in npm.

Looking at the rollup config, I can see the UMD output of rollup if configured as a “Immediately Invoked Function Expression”, which would work for the browser with globals but is not a UMD module: https://github.com/pixijs/pixi-tilemap/blob/4c9dbdb65b71fc26390ef3311e07a628a9b0044b/rollup.config.js#L93-L97

Is this expected? 😃

Changing this to “umd” and removing the footer seems to properly generate the UMD canonical code:

function (global, factory) {
    typeof exports === 'object' && typeof module !== 'undefined' ? factory(exports, require('@pixi/display'), require('@pixi/core'), require('@pixi/constants'), require('@pixi/math'), require('@pixi/graphics'), require('@pixi/sprite')) :
    typeof define === 'function' && define.amd ? define(['exports', '@pixi/display', '@pixi/core', '@pixi/constants', '@pixi/math', '@pixi/graphics', '@pixi/sprite'], factory) :
    (global = typeof globalThis !== 'undefined' ? globalThis : global || self, factory(global.pixi_tilemap = {}, global.PIXI, global.PIXI, global.PIXI, global.PIXI, global.PIXI, global.PIXI));
}(this, (function (exports, display, core, constants, math, graphics, sprite) { 'use strict';

Unrelated: @blurymind please note that for GDevelop I’ll need to modify the loader code to ensure all the requires (require('@pixi/display'), require('@pixi/core'), require('@pixi/constants')) are working. For now, we only allow require("pixi.js") and require("pixi.js-legacy") to be used. I’ll probably have to alias all the @pixi/....

OK, I removed PIXI.xxx references, 2.1.2 , please try again

Thanks! 👍 I can still see 2 PIXI.CanvasRenderer, and 1 PIXI.utils.createIndicesForQuads, should they be replaced too?

The whole point of importing from @pixi/ modules is so that we don’t have a dependency on the pixi.js bundle.

Ah I see! Forget about it then.

I think a better solution would be to check if pixi_tilemap exists (which means it is in a browser):

Yeah that seems fair and way less complicated than my global stuff. Let’s go ahead with this, I don’t see any reason why it would not work with this extra guard.

I guess we should also surely remove what is done in exporter.ts? It’s assigning pixi_tilemap to PIXI.tilemap, but the generated code seems not to done anything good:

    var pixi_tilemap;
    (function (pixi_tilemap) {
        PIXI.tilemap = pixi_tilemap;
    })(pixi_tilemap || (pixi_tilemap = {}));
    var exporter = {};

The issue being that:

  • it uses the global PIXI,
  • it assign something to PIXI.tilemap, but this something is always an empty object.

Seems that this will be taken care of by the updated assign suggested by @SukantPal?

Then hopefully we’re good in having a well encapsulated, non leaking, but-still-working-in-the-browser-and-augmenting-PIXI-global-in-this-case module 🤞🤞

@4ian The whole point of importing from @pixi/ modules is so that we don’t have a dependency on the pixi.js bundle.

I think a better solution would be to check if pixi_tilemap exists (which means it is in a browser):

if (typeof pixi_tilemap !== 'undefined) {
  Object.assign(this.PIXI.tilemap, pixi_tilemap);
}

Note that this.PIXI.tilemap is ensured to be defined in the beginning of the file.

Sorry I edited my message after you answered 😃 This line is problematic because it’s not working when requiring this module as CommonJS: pixi_tilemap is not defined in this context, because we’re outside the function embedding the library.

I understand that plugins will register themselves into PIXI, here in PIXI.tilemap, but currently this is erroring because the global variable pixi_tilemap is not defined when you run this module in CommonJS (or in AMD from what i’ve seen). Only the path for the browser will work (because the object global.pixi_tilemap = {} is created then passed to the function embedding the library as the first parameter, called exports).

Seems like there is still an extra line at the end of the UMD module (problematic because pixi_tilemap is not defined in this context, because we’re outside the function embedding the library): https://github.com/pixijs/pixi-tilemap/blob/2b7fb150cc867da88bc1631d163f71c9b3b54190/dist/pixi-tilemap.umd.js#L961

Apart from that, seems like a working module 😃

Ah I see, I was assuming Pixi.js was UMD/CommonJS as it can be properly required when installed from npm.

This being said, I see that now the umd output seems to be properly a UMD module: https://github.com/pixijs/pixi-tilemap/blob/master/dist/pixi-tilemap.umd.js since your commit https://github.com/pixijs/pixi-tilemap/commit/9c9c1c73424b27bb2b5b053d6fe0208037ba2c20 to use @pixi-build-tools/rollup-configurator 😃 Some seems we’re now good?

The output is not “totally orthodox” because things are still assigned to global PIXI and PIXI.Tilemap objects, but I guess it’s no problem for us (apart from polluting a bit the globals in Electron, it should still work, and should still work in the browser). Time to give this another try I think @blurymind 😃 (We might have another issue pending but unrelated to the build I think)