Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UMD module build does not have a requirejs export #97

Open
blurymind opened this issue Aug 21, 2020 · 26 comments
Open

UMD module build does not have a requirejs export #97

blurymind opened this issue Aug 21, 2020 · 26 comments

Comments

@blurymind
Copy link
Contributor

blurymind commented Aug 21, 2020

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
4ian/GDevelop#1901

@4ian
Copy link

4ian commented Aug 21, 2020

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/....

@ShukantPal
Copy link
Contributor

I'm afraid that pixi.js itself doesn't output to UMD, however.

@ShukantPal
Copy link
Contributor

@4ian Why aren't you using CommonJS for Electron instead of UMD?

@4ian
Copy link

4ian commented Aug 25, 2020

We are, but I figured out UMD would be convenient as it's for example what is used for pixi-multistyle-text (see the generated file here) and it has the advantage of working:

  • In Electron (we "just" require it, and get back MultiStyleText that we can then use to render it in the editor).
  • In the browser, in which case it's exported as a global, that we can use directly. This is what we use for the game engine (we don't have any bundling for the game engine).

So I was supposing UMD would be the more convenient way of exporting a module, as it works in CommonJS and as a browser global (and as a AMD require) with a single file :)
Maybe I missed something though that would make impossible/not satisfactory to have this module as UMD?

If really required, we can always embed two files for pixi-tilemap, one as a browser global (used by the game engine), and another one as a CommonJS module (for the editor).

@ShukantPal
Copy link
Contributor

ShukantPal commented Aug 25, 2020 via email

@4ian
Copy link

4ian commented Aug 25, 2020

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 9c9c1c7 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)

@blurymind
Copy link
Contributor Author

I will give this another try in gdevelop when I get some time :)

@ivanpopelyshev
Copy link
Collaborator

All done, please try again.

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

@4ian
Copy link

4ian commented Aug 29, 2020

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 :)

@ShukantPal
Copy link
Contributor

ShukantPal commented Aug 29, 2020 via email

@4ian
Copy link

4ian commented Aug 29, 2020

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).

@bpkennedy
Copy link

bpkennedy commented Aug 29, 2020

Can confirm that the 2.1.1 npm package simply isn't exporting the library correctly. I rolled back to npm version 2.0.6 and it works.

I am not using whatever pixi bundling/rollup is mentioned a couple times elsewhere. I am simply getting npm packages and doing inside a Vue project:

import * as PIXI from 'pixi'
...
window.PIXI = PIXI // this is needed for some reason??
require('pixi-tilemap') // this MUST happen after importing PIXI and assigning to window

Versions:
"pixi-tilemap": "^2.0.6",
"pixi.js": "^5.3.3",

@ShukantPal
Copy link
Contributor

@4ian Should it then be

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

@ShukantPal
Copy link
Contributor

@bpkennedy If you are using import/export, you now simply

import { RectTileLayer } from 'pixi-tilemap'

directly.

@4ian
Copy link

4ian commented Aug 30, 2020

There seems to be two problems:

  1. The one highlighted by @bpkennedy: a global object called PIXI is still required. This is because there are still references to "PIXI" in the generated UMD module - which seems like mistakes/forgotten references. One example:

In the UMD build:
https://github.com/pixijs/pixi-tilemap/blob/430adfc6f186f1bdaa55cccd7f5ce4a69570dabf/dist/pixi-tilemap.umd.js#L49
this is coming from this source that is indeed referencing PIXI:
https://github.com/pixijs/pixi-tilemap/blob/430adfc6f186f1bdaa55cccd7f5ce4a69570dabf/src/RectTileLayer.ts#L3-L22

The solution is I believe to remove these PIXI.Xyz to replace them by a import { Xyz } from '@pixi/...'; (I'm not super familiar with how is modularized Pixi, but seems that it's working well for the rest).
Should be doable by just searching for PIXI. in the sources.

  1. Second problem is that while we're bundling as a UMD, we still want in the case of the browser globals (i.e: when we're not in an AMD environment and not in a CommonJS environment) to define PIXI.tilemap to be what is exported as global.pixi_tilemap.

Seems like this is handled by this code in the rollup configurator but as we've seen, it would be failing in the case of CommonJS/AMD.

I think there are two solutions:

  • Get the global PIXI to be a dependency, so that it's passed to the module and we can set PIXI.tilemap = pixi_tilemap, with pixi_tilemap being what is defined here.
    • For this, I guess it's a matter of adding an import * as PIXI from 'pixi.js' in index.ts? Then do PIXI.tilemap = pixi_tilemap. In a way this is what was kind of done in exporter.ts, but I think that what exporter.ts is doing is useless.
  • The other solution: rework the Object.assign. Instead of window, I would rather use global:
if (typeof global.PIXI !== 'undefined' && typeof global.pixi_tilemap !== 'undefined') {
  Object.assign(global.PIXI.tilemap, global.pixi_tilemap);
}

but I'm not entirely sure, because playing with global is risky (the generated UMD code goes a great length for global, doing: global = typeof globalThis !== 'undefined' ? globalThis : global || self, which might be to be compatible with web workers).

I would rather try the first solution (import * as PIXI from 'pixi.js' and then assign PIXI.tilemap) first, as it would make a UMD module with "no hacks" (just am assignement to PIXI, which is fine as it's done inside the UMD module).

@ivanpopelyshev
Copy link
Collaborator

ivanpopelyshev commented Aug 30, 2020

OK, I removed PIXI.xxx references, 2.1.2 , please try again. I dont understand what do you want about PIXI.tilemap namespace.

Guys, I just remind you that I only slightly understand whats going on :)

@ShukantPal
Copy link
Contributor

@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.

@4ian
Copy link

4ian commented Aug 30, 2020

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 🤞🤞

@ShukantPal
Copy link
Contributor

@4ian We should be done here?

@blurymind
Copy link
Contributor Author

blurymind commented Sep 1, 2020

@ivanpopelyshev @4ian @SukantPal thanks everyone for getting involved :) with your help pixi-tilemap is getting closer to be a part of gdevelop.

I will soon be able to also confirm it works ok on the web version of gdevelop. My ultimate hope is that we dont need to use a patched library in GD or if its patched its minimal

@4ian
Copy link

4ian commented Sep 1, 2020

Thanks all!
Though @blurymind could you be able to try the updated library (replace it, then re-run npm start or at least npm run import-resources)? I'm still worried of:

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

throwing an undefined reference to pixi_tilemap when used in Electron (despite the guard).
If it's working well with the UMD file as is, we can close the issue, just want to double check that.

@spassvogel
Copy link
Contributor

I used to be able to

window.PIXI = PIXI;
import 'pixi-tilemap';

and then
const tileLayer = new window.PIXI.tilemap.CompositeRectTileLayer();

in a typescript project. That doesn't work anymore.

I tried to import { RectTileLayer } from 'pixi-tilemap'. That compiles, but it doesn't render anything :( I'm using 2.1.3.

@ShukantPal
Copy link
Contributor

@spassvogel Can you give us a reproduction? Are there any errors being thrown? Also, be sure to check if pixi-tilemap is not getting its own copy of @pixi/core (or any other pixi package).

@spassvogel
Copy link
Contributor

spassvogel commented Sep 6, 2020

@spassvogel Are there any errors being thrown?

No errors. just nothing rendered.

Also, be sure to check if pixi-tilemap is not getting its own copy of @pixi/core (or any other pixi package).

How would I see that?

Can you give us a reproduction?

Yes okay I can set up a test project

@ShukantPal
Copy link
Contributor

@spassvogel You can see if pixi-tilemap gets its own copy by: opening node_modules/pixi-tilemap/node_modules and see if there is a @pixi package there.

@spassvogel
Copy link
Contributor

spassvogel commented Sep 6, 2020

Well I tried to create an isolated case but typescript is complaining about missing typedefs


ERROR in [at-loader] ./node_modules/pixi-tilemap/index.d.ts:1:29 
    TS7016: Could not find a declaration file for module '@pixi/core'. 'repositories/pixi-tilemap-test/node_modules/@pixi/core/lib/core.js' implicitly has an 'any' type.
  Try `npm install @types/pixi__core` if it exists or add a new declaration (.d.ts) file containing `declare module '@pixi/core';`

ERROR in [at-loader] ./node_modules/pixi-tilemap/index.d.ts:2:24 
    TS7016: Could not find a declaration file for module '@pixi/display'. 'repositories/pixi-tilemap-test/node_modules/@pixi/display/lib/display.js' implicitly has an 'any' type.
  Try `npm install @types/pixi__display` if it exists or add a new declaration (.d.ts) file containing `declare module '@pixi/display';`

ERROR in [at-loader] ./node_modules/pixi-tilemap/index.d.ts:3:36 
    TS7016: Could not find a declaration file for module '@pixi/core'. 'repositories/pixi-tilemap-test/node_modules/@pixi/core/lib/core.js' implicitly has an 'any' type.
  Try `npm install @types/pixi__core` if it exists or add a new declaration (.d.ts) file containing `declare module '@pixi/core';`

ERROR in [at-loader] ./node_modules/pixi-tilemap/index.d.ts:4:27 
    TS7016: Could not find a declaration file for module '@pixi/display'. 'repositories/pixi-tilemap-test/node_modules/@pixi/display/lib/display.js' implicitly has an 'any' type.
  Try `npm install @types/pixi__display` if it exists or add a new declaration (.d.ts) file containing `declare module '@pixi/display';`

ERROR in [at-loader] ./node_modules/pixi-tilemap/index.d.ts:5:26 
    TS7016: Could not find a declaration file for module '@pixi/core'. 'repositories/pixi-tilemap-test/node_modules/@pixi/core/lib/core.js' implicitly has an 'any' type.
  Try `npm install @types/pixi__core` if it exists or add a new declaration (.d.ts) file containing `declare module '@pixi/core';`

ERROR in [at-loader] ./node_modules/pixi-tilemap/index.d.ts:6:27 
    TS7016: Could not find a declaration file for module '@pixi/core'. 'repositories/pixi-tilemap-test/node_modules/@pixi/core/lib/core.js' implicitly has an 'any' type.
  Try `npm install @types/pixi__core` if it exists or add a new declaration (.d.ts) file containing `declare module '@pixi/core';`

ERROR in [at-loader] ./node_modules/pixi-tilemap/index.d.ts:7:26 
    TS7016: Could not find a declaration file for module '@pixi/graphics'. 'repositories/pixi-tilemap-test/node_modules/@pixi/graphics/lib/graphics.js' implicitly has an 'any' type.
  Try `npm install @types/pixi__graphics` if it exists or add a new declaration (.d.ts) file containing `declare module '@pixi/graphics';`

ERROR in [at-loader] ./node_modules/pixi-tilemap/index.d.ts:8:24 
    TS7016: Could not find a declaration file for module '@pixi/math'. 'repositories/pixi-tilemap-test/node_modules/@pixi/math/lib/math.js' implicitly has an 'any' type.
  Try `npm install @types/pixi__math` if it exists or add a new declaration (.d.ts) file containing `declare module '@pixi/math';`

ERROR in [at-loader] ./node_modules/pixi-tilemap/index.d.ts:9:32 
    TS7016: Could not find a declaration file for module '@pixi/core'. 'repositories/pixi-tilemap-test/node_modules/@pixi/core/lib/core.js' implicitly has an 'any' type.
  Try `npm install @types/pixi__core` if it exists or add a new declaration (.d.ts) file containing `declare module '@pixi/core';`

ERROR in [at-loader] ./node_modules/pixi-tilemap/index.d.ts:10:27 
    TS7016: Could not find a declaration file for module '@pixi/math'. 'repositories/pixi-tilemap-test/node_modules/@pixi/math/lib/math.js' implicitly has an 'any' type.
  Try `npm install @types/pixi__math` if it exists or add a new declaration (.d.ts) file containing `declare module '@pixi/math';`

ERROR in [at-loader] ./node_modules/pixi-tilemap/index.d.ts:11:26 
    TS7016: Could not find a declaration file for module '@pixi/core'. 'repositories/pixi-tilemap-test/node_modules/@pixi/core/lib/core.js' implicitly has an 'any' type.
  Try `npm install @types/pixi__core` if it exists or add a new declaration (.d.ts) file containing `declare module '@pixi/core';`

ERROR in [at-loader] ./node_modules/pixi-tilemap/index.d.ts:12:27 
    TS7016: Could not find a declaration file for module '@pixi/core'. 'repositories/pixi-tilemap-test/node_modules/@pixi/core/lib/core.js' implicitly has an 'any' type.
  Try `npm install @types/pixi__core` if it exists or add a new declaration (.d.ts) file containing `declare module '@pixi/core';`

ERROR in [at-loader] ./node_modules/pixi-tilemap/index.d.ts:13:29 
    TS7016: Could not find a declaration file for module '@pixi/constants'. 'repositories/pixi-tilemap-test/node_modules/@pixi/constants/lib/constants.js' implicitly has an 'any' type.
  Try `npm install @types/pixi__constants` if it exists or add a new declaration (.d.ts) file containing `declare module '@pixi/constants';`

ERROR in [at-loader] ./node_modules/pixi-tilemap/index.d.ts:14:24 
    TS7016: Could not find a declaration file for module '@pixi/core'. 'repositories/pixi-tilemap-test/node_modules/@pixi/core/lib/core.js' implicitly has an 'any' type.
  Try `npm install @types/pixi__core` if it exists or add a new declaration (.d.ts) file containing `declare module '@pixi/core';`

ERROR in [at-loader] ./node_modules/pixi-tilemap/index.d.ts:15:24 
    TS7016: Could not find a declaration file for module '@pixi/sprite'. 'repositories/pixi-tilemap-test/node_modules/@pixi/sprite/lib/sprite.js' implicitly has an 'any' type.
  Try `npm install @types/pixi__sprite` if it exists or add a new declaration (.d.ts) file containing `declare module '@pixi/sprite';`

ERROR in [at-loader] ./node_modules/pixi-tilemap/index.d.ts:16:25 
    TS7016: Could not find a declaration file for module '@pixi/core'. 'repositories/pixi-tilemap-test/node_modules/@pixi/core/lib/core.js' implicitly has an 'any' type.
  Try `npm install @types/pixi__core` if it exists or add a new declaration (.d.ts) file containing `declare module '@pixi/core';`
ℹ 「wdm」: Failed to compile.

However these packages dont exist..

https://github.com/spassvogel/pixi-tilemap-test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants