mrdoob / three.js

JavaScript 3D Library.
https://threejs.org/
MIT License
102.67k stars 35.37k forks source link

Importing examples jsm modules causes bundlers to bundle three.js source code twice #17482

Closed adrian-delgado closed 3 years ago

adrian-delgado commented 5 years ago

Importing from three/examples/jsm/.../<module> causes bundlers (tested with rollup) to include the library twice (or multiple times).

For example, when doing import { OrbitControls } from 'three/examples/jsm/controls/OrbitControls', the bundler will follow the import and in the OrbitControls.js the imports come from ../../../build/three.module.js. However, there is no way for the (external) bundler to know that ../../../build/three.module.js is the same module as three.

A solution for this would be to treat the examples modules as external packages and import from three instead of ../../../build/three.module.js. This might break the rollup config of three.js, but it should be possible to tell rollup that three is an alias for the main entry point of three (src/Three.js).

mrdoob commented 3 years ago

I'm not sure how this would affect cdn use like unpkg.com, though.

Oh man, I was excited about the idea until I reached that line πŸ˜…

It looks like unpkg.com specifically supports a ?module url parameter to automatically resolve the bare import specifiers but it is marked as experimental on the home page.

And I guess it would also break https://cdnjs.cloudflare.com/ πŸ˜•

gkjohnson commented 3 years ago

Oh man, I was excited about the idea until I reached that line πŸ˜…

Heh I figured that might be the case. Depending on what exactly your hesitations are switching unpkg references over to use https://skypack.dev/ would be an option because it already supports handling bare module imports and otherwise behaves very similar to unpkg.

In practice this means changing lines like this:

import * as THREE from 'https://unpkg.com/three/build/three.module.js'

to this:

import * as THREE from 'https://cdn.skypack.dev/three'

Here's a quick demo in JSFiddle demonstrating that it "just worksβ„’" (as far as I can tell) using my three-mesh-bvh package which uses bare module imports to import "three":

https://jsfiddle.net/2zp3nduj/1/

And I guess it would also break https://cdnjs.cloudflare.com/ πŸ˜•

Yes but when pushing to cdnjs you should push the versions of the modules that refer to three.module.js directly rather than bare modules. I don't expect any "dumb" CDN will ever be able to work with bare modules until something like import maps are available.

donmccurdy commented 3 years ago

I don't think Cloudflare hosts our examples at all, just the core library: https://cdnjs.com/libraries/three.js. And since embedding examples hosted somewhere else would bring in another copy, that's very fragile even today.

As @gkjohnson, Skypack supports bare imports (e.g. three) really well. Less "smart" CDNs like unpkg or JSDelivr could not, to my understanding.

Blakeinstein commented 3 years ago

How about having a seperate npm package for examples only for end users that has three js as a dependency?

It could also have cdn variants that assumes three js has been already imported..

donmccurdy commented 3 years ago

How about having a separate npm package for examples only for end users that has three js as a dependency?

Or to riff on this idea a bit, maybe a pre-publish script that generates a new directory within the existing npm package? It's only the examples/jsm/* code that needs to be transformed here. In that case we'd have something like:

The latter could be generated by a pre-publish script and not checked into the repository.

Could also (needs more research?) set up an exports map so that bundlers automatically resolve a shorthand path, like:

import { OrbitControls } from 'three/controls/OrbitControls.js';

It could also have cdn variants that assumes three js has been already imported..

That's essentially what examples/js/* provides, but it isn't compatible with ES modules. I don't think we want to encourage a workflow that mixes global variables with ES modules.

mrdoob commented 3 years ago

@gkjohnson @donmccurdy

I think I'm all for adopting https://skypack.dev/ and having a script that converts ".../build/three.module.js" to "three" at npm publish time πŸ‘

mrdoob commented 3 years ago

Just to clarify... The examples would continue using ".../build/three.module.js" but because in npm we'd be using "three" people using jsfiddle, codepen, glitch, ... would have to use https://skypack.dev/ instead of https://unpkg.com/.

Mugen87 commented 3 years ago

I think that's a good compromise (considering that we can finally solve this issue^^).

donmccurdy commented 3 years ago

Just to confirm, we'd be transforming the examples/jsm folder at publish time β€” not creating a 3rd directory β€” such that it's compatible with skypack and modern bundlers, with the understanding that unpkg and older CDNs won't work for examples/jsm in future releases?

Could we do an "alpha" release with these changes first, to confirm everything works as expected on skypack? Would be unfortunate to drop unpkg support and then find that skypack is not working as intended, although I don't expect that.

(/cc @drcmda since you'd brought up skypack before)

gkjohnson commented 3 years ago

Just made PR #21654 with a prepublish script.

@donmccurdy

just to confirm, we'd be transforming the examples/jsm folder at publish time β€” not creating a 3rd directory

I would be concerned that publishing a third folder would cause confusion with some people pulling from the wrong folder etc.

with the understanding that unpkg and older CDNs won't work for examples/jsm in future releases?

Which other CDNs will break? Only CDNs that pull directly from NPM will break. Are there others? I would think that most CDNs that pull from NPM should be bare-module aware considering that's the predominant way to distribute packages there. I like unpkg but I expect it to break in a lot of cases like the example I posted above and I think it's a problem that it doesn't support bare modules more robustly.

Could we do an "alpha" release with these changes first, to confirm everything works as expected on skypack?

What are you thinking might break? I'm not against a test run of some kind but I'm also thinking that if it doesn't work for some reason a point release could be made with reverted paths.

Could also (needs more research?) set up an exports map so that bundlers automatically resolve a shorthand path, like:

I like this idea. It looks like we could just add this to package.json:

"exports": {
    "./": "./examples/jsm/"
}
donmccurdy commented 3 years ago

Which other CDNs will break? Only CDNs that pull directly from NPM will break. Are there others? I would think that most CDNs that pull from NPM should be bare-module aware considering that's the predominant way to distribute packages there.

I think https://www.skypack.dev/ is one of a kind at this point. Comparing CDNs I'm familiar with:

What are you thinking might break?

I'm not sure what will happen with UMD packages, e.g. dependencies in examples/js/libs/*, like draco_decoder.js. Maybe you just have to get these from another CDN? Maybe they're left untouched by skypack?

Similarly, I don't know what chevrotain.module.min.js is, it seems to be mixing and matching UMD and ES Module syntax.

I like this idea. It looks like we could just add [pkg.exports] to package.json:

I think I'd read somewhere that once you define pkg.exports, all unlisted entrypoints are blocked (at least by Node.js?) so that is the "needs more research" concern. πŸ€”

gkjohnson commented 3 years ago

Jsdelivr: Static file server.

Oh I actually didn't realize jsdelivr could pull from npm. It looks like it can pull from Github, as well, so there's a relatively easy work around in this case.

I'm not sure what will happen with UMD packages, e.g. dependencies in examples/js/libs/*, like draco_decoder.js. Maybe you just have to get these from another CDN? Maybe they're left untouched by skypack?

Similarly, I don't know what chevrotain.module.min.js is, it seems to be mixing and matching UMD and ES Module syntax.

Just looking at the chevrotain module on skypack it looks good. It still just sets chevrotain on window and exports it. These cases could be tested in their current form, though, by modifying an example that imports it to point to skypack in a local repo.

It does look like draco_decoder.js might be a problem, though. It looks like skypack tries to convert the require statements to use import syntax but chokes on the "path" and "fs" requires used conditionally in the middle of the emscripten-converted code and will throw an error when it tries to import them. Not exactly sure what to do about this case -- it's a bit of a weird one.

I think I'd read somewhere that once you define pkg.exports, all unlisted entrypoints are blocked (at least by Node.js?) so that is the "needs more research" concern. πŸ€”

Ha okay that's a good point 😁

donmccurdy commented 3 years ago

Not exactly sure what to do about this case -- it's a bit of a weird one.

Ok, thanks for checking! I don't think it should prevent us from going ahead with this plan (emscripten has lots of compile options for these situations) but does mean a bit more to test.

Mugen87 commented 3 years ago

Should be fixed with r128! πŸŽ‰

mrdoob commented 3 years ago

Just to clarify, the current solution doesn't yet work with https://unpkg.com/ (needs ?module). People should use https://www.skypack.dev/ instead.

Mugen87 commented 3 years ago

I've added a note in the migration guide for this. Feel free to add additional information if something is missing:

https://github.com/mrdoob/three.js/wiki/Migration-Guide#127--128

mrdoob commented 3 years ago

Thanks! Did a bit of clean up πŸ‘

mmncit commented 3 years ago

Should be fixed with r128! πŸŽ‰

Updated the version of "three": "^0.128.0" and "@types/three": "^0.127.1" in package.json. But, still observing the **WARNING: Multiple instances of Three.js being imported**. while running the test.

Question: Does any jest configuration (here) need to be taken care of?

alexravenna commented 3 years ago

Should be fixed with r128! πŸŽ‰

Updated the version of "three": "^0.128.0" and "@types/three": "^0.127.1" in package.json. But, still observing the **WARNING: Multiple instances of Three.js being imported**. while running the test.

Question: Does any jest configuration (here) need to be taken care of?

I have the same issue!

marcofugaro commented 3 years ago

Please share a minimal reproducible example.

taseenb commented 3 years ago

For those using Webpack, this solution still works (on both Webpack 4 and 5) after the r128 fix: https://github.com/mrdoob/three.js/issues/17482#issuecomment-780920930

Basically it won't include three in your build (so you can use a CDN, for example) but will include the examples. It works because the imports in the examples are now "three/*" but we only ignore "three". The part related to three.module.js can probably be removed now.

jashson commented 2 years ago

The issue came back with r137 using webpack (at least for me). With versions lower then r137 i do not get any warnings of multiple instances of three.js. So the solution is currently to stick to r136. I tried r138 which has the same issue like r137

marcofugaro commented 2 years ago

@jashson could you share your configuration/import? does not happen for me

jashson commented 2 years ago

@marcofugaro sure - i use webpack v5.69.1 and webpack-dev-server v4.7.4. with TypeScript. Three.js is available via npm / package.json. (no CDN)

Here is the essential part of my webpack.config.js

module.exports = {
  entry: {
    js: {import: 'index.ts', filename: 'app.js'},
  },
  ...
}

And here the import of Three.js and OrbitControls:

import {
    PerspectiveCamera,
    Scene,
    WebGLRenderer
} from "three";
import {OrbitControls} from "three/examples/jsm/controls/OrbitControls";
marcofugaro commented 2 years ago

@jashson did a minimal repro with the code you provided and the issue does not happen for me:

webpack-three-test.zip

It's a problem with your personal configuration, the place for this issue is over at stackoverflow.

jashson commented 2 years ago

@marcofugaro Thanks you for your help and the minimal repro (which works just fine). I'll will check my configuration.

Zundrium commented 2 years ago

@jashson I'm facing the same issue, would appreciate if you could share your solution :)

jashson commented 2 years ago

@Zundrium I'm currently in a kind of production-hell, so that I had no time to look for the reason in my configuration. My quick solution is to use r136, which runs without warnings.

Zundrium commented 2 years ago

@jashson Alright, thanks for the quick response!