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

ReactAreaLights do not seem to work in a module bundler #17220

Closed
3 tasks done
drcmda opened this issue Aug 9, 2019 · 50 comments
Closed
3 tasks done

ReactAreaLights do not seem to work in a module bundler #17220

drcmda opened this issue Aug 9, 2019 · 50 comments
Labels

Comments

@drcmda
Copy link
Contributor

drcmda commented Aug 9, 2019

Description of the problem

basically, area lights do not work with npm/node module resolution, see:

simplified: https://codesandbox.io/s/three-fibre-userender-test-rohv5

raw threejs arealight demo: https://codesandbox.io/s/dreamy-platform-3jpim

why

all files under examples/jsm pull from ../../../build/three.module.js instead of three, which is arguably not correct. it will pull threejs two times in bundling systems that do not follow package.json:module (but "main" instead, which leads to build/three.js). it then creates two separate namespaces. uniforms.init() ends up writing to the wrong one.

solution

this is how a typical jsm file should look:

import {
	ClampToEdgeWrapping,
	DataTexture,
	FloatType,
	LinearFilter,
	NearestFilter,
	RGBAFormat,
	ShaderLib,
	UVMapping,
	UniformsLib
} from "three";

that's how any other library works. i have never seen library that relies on a distro file - this has obvious pitfalls. i understand that this was made to make the html demos work, but imo this is a hack, and it now impacts production usage.

Three.js version
  • r107
Browser
  • Chrome
OS
  • macOS
@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 9, 2019

it looks like this file: https://github.com/mrdoob/three.js/blob/master/examples/jsm/lights/RectAreaLightUniformsLib.js is mutating a namespace.

I'm not sure I understand. Can you please explain in more detail what you mean?

It seems to work fine in the official example:

https://threejs.org/examples/webgl_lights_rectarealight

@drcmda
Copy link
Contributor Author

drcmda commented Aug 9, 2019

PS. i have included a console.log right after the init() function, which writes ltc_1 and 2 into the shaderlib. both ltc_1 and 2 are undefined, because three/Shaderlib and three.module/Shaderlib are two separate modules.

i think this is actually worse, i hope i'm wrong but i believe that any project right now that relies on examples/jsm pulls two full threejs bundles.

It works if you do:

import * as THREE from 'three/build/three.module'

but that's no solution because 100% of all three extension in the eco system pull from 'three'.

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 9, 2019

i think this is actually worse, i hope i'm wrong but i believe that any project right now that relies on examples/jsm pulls two full threejs bundles.

I think this is not true. As you can see at the following repository, you can perform ES6 imports like so...

import {
	BoxBufferGeometry,
	Mesh,
	MeshBasicMaterial,
	PerspectiveCamera,
	Scene,
	WebGLRenderer
} from 'three';

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

...and the resulting bundle (created with rollup) does not contain multiple version of three.js. https://github.com/Mugen87/three-jsm

It seems to me you are doing something wrong in your build or at codebox.

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 9, 2019

@Mugen87 Mugen87 added the Addons label Aug 9, 2019
@drcmda
Copy link
Contributor Author

drcmda commented Aug 9, 2019

this happens in all bundlers i've tried. create-react-app, webpack, codesandbox, parcel, etc. it imports from a namespace that differs from "three", so these mutations end up in another space.

your example works because rollup happens to pull from three.module via package.json:jsnext:main/module. if it would follow "main" or fetch three from source, you would have two threejs versions in the output and arealights would also not work.

jsm/examples like a normal eco system lib. it should refer to "three", not cling to a particular build file. this is the root cause and it's definitively not correct. is there any reason why it does that, though?

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 9, 2019

your example works because rollup happens to pull from three.module via package.json:jsnext:main/module. if it would follow "main" or fetch three from source, you would have two threejs versions in the output and arealights would also not work.

I think rollup does it the correct way and fetches three from the intended location (like configured in package.json). I'm not familiar with the other bundlers but I suppose it's a matter of configuration.

is there any reason why it does that, though?

three.js is not a pure node.js project. It's intended that you can directly load modules in HTML pages like in the official examples. If we would load modules from 'three', a dependency resolution is not possible.

@njarraud
Copy link
Contributor

njarraud commented Aug 9, 2019

I do not understand the issue either. I use the node system located in examples/jsm and build my app with webpack. Everything works as intended and I do not have any problem of double threejs import.

@drcmda
Copy link
Contributor Author

drcmda commented Aug 9, 2019

@njarraud im more concerned with react area lights. you can see in both demos above that it doesn't work. webpack has the same problems once it pulls from main.

three.js is not a pure node.js project. It's intended that you can directly load modules in HTML pages like in the official examples. If we would load modules from 'three', a dependency resolution is not possible.

this is a real problem. i think this lib should work with the tools that everyone uses (npm). but relying on a particular dist file is not good practice in general. it works in that html file now, but it impacts the usefulness of jsm. also, just like that the commonjs export is incompatible with examples/jsm.

@drcmda
Copy link
Contributor Author

drcmda commented Aug 9, 2019

tried to search through sources,

html demo

Screenshot 2019-08-10 at 00 08 21

bundled

Screenshot 2019-08-10 at 00 08 59

as i suspected it pulls both three.js and three.module.js and creates two namespaces. you can assume this has silently happened in every bundling system that doesn't follow the module field.

i think it could also at least affect the html demos if they pull threejs 3rd party npm packages, which all resolve "three".

@njarraud
Copy link
Contributor

njarraud commented Aug 9, 2019

this is a real problem. i think this lib should work with the tools that everyone uses (npm).

The reason I like and use threejs is because it doesn't depend on any particular third party platform like Nodejs. That is a strength from my point of view.

I'll check if I have an issue with ReactAreaLights as well.

@drcmda
Copy link
Contributor Author

drcmda commented Aug 9, 2019

doesn't depend on any particular third party platform like Nodejs. That is a strength from my point of view.

That still does not make it OK to break it for the all the people that use it. Standard imports are still arguably useless, as is evident by this very issue: it couldn't just use "three". Spec imports depend on a build system because they don't have a resolution concept yet. But in any case, the root cause is a misunderstanding of how modules behave. Linking a distro file isn't something you would do, bundlers or spec imports.

@njarraud
Copy link
Contributor

njarraud commented Aug 9, 2019

That still does not make it OK to break it for the all the people that use it.

It doesn't make it OK either to break it for people who don't use it. I leave it to @mrdoob and the other three.js guys as they are definitely more qualify than I am to answer you concerns.

@gkjohnson
Copy link
Collaborator

gkjohnson commented Aug 10, 2019

I'm using webpack with babel, react, three.js, and a variety of three.js example modules and I'm not seeing a any duplicate threejs bundles included. I set up two small example builds using rollup and parcel and both seemed to work just fine, as well.

if it would follow "main" or fetch three from source, you would have two threejs versions in the output and arealights would also not work.

Can you describe the setup you're using such that the build process is using the build pointed to by package.json "main"? And by fetching from source do you mean pointing to the "src/Three.js" file? My experience is that the bundlers prioritize the "module" field over "main" so I wouldn't expect that this would be a problem.

I expect that the solution won't be to immediately change all the example scripts over to import from "three" because that would break quite a few things -- maybe we can talk about solutions that afford both use cases instead?

@drcmda
Copy link
Contributor Author

drcmda commented Aug 10, 2019

@gkjohnson Here's the statement from Ives, he made csb: https://twitter.com/0xca0a/status/1159954977672105984

It's up to the bundler to decide which it pulls and both module/main are permissable. If a library on the other hand imports from "three/build/three.module", that is not correct, we're skipping resolution now. all bundlers that follow main will have two bundles and all interop will break. this will also happen in webpack. that is the reason we have these fields in package.json in the first place, for resolution.

i think it's the demos that have to be adjusted, not the module standard as it stands.

in a real world project there would always be labour involved if you want to deal with browser imports, you would have a dependencies folder inside source and any package you use has to go at least through rollup to resolve to it. the spec is far from being practicable.

@looeee
Copy link
Collaborator

looeee commented Aug 10, 2019

I've bundled three.js with webpack on a couple of projects and never noticed this happening. This sounds like it may be an issue with codesandbox.

@drcmda, could you create a standalone package in a zip file using webpack or another bundler, so that we can run it locally and verify the issue?

@gkjohnson
Copy link
Collaborator

It's up to the bundler to decide which it pulls and both module/main are permissable.
all bundlers that follow main will have two bundles and all interop will break

I didn't expect this to be happening in practice -- all testing and experience indicates that module is being used over main. I see that at least CSB is using main, though. It would still be nice to know in what cases this happens with different bundlers.

I'll be the first to agree that the state of javascript imports is a bit of a mess and I don't think it's always clear what the "best" way to provide packages is right now. Because the examples reference a file they can be used directly in a browser through rawgit, unpkg, and just served with a static server for dev -- I feel that ease of use is something that's defined the library for awhile, now. I don't want to ignore the problem you've brought up but I do think it would be good to understand how common the issue is.

I also think it's most productive to discuss solutions to the problem, as well, so any thoughts you have in that vein would be appreciated. If the example modules are going to be switched over to use "three" then there will have to be some plan to keep the examples working and testable via rawgit.

@drcmda
Copy link
Contributor Author

drcmda commented Aug 14, 2019

I feel that ease of use is something that's defined the library for awhile, now. I don't want to ignore the problem you've brought up but I do think it would be good to understand how common the issue is.

That's my main concern. Is anyone here using browser imports? And how? You would not be able to use any package or npm depedency. The spec is merely theoretic at this point and riddled with problems that kind of make adoption impossible atm. That is why it is still a staged draft, not a standard, nothing should rely on it.

Npm on the other hands is used by the entire world. But three still has a problem with modules, as it does things nothing else does. As it stands, the examples collection breaks the module resolution, and that directly goes against what you said, ease of use.

We have a working html demo, but no one would ever be able to use any of it in a real project. But if you use three in the real world, it can break. That's not a good compromise imo. And from a technical standpoint, relying on a distro module would also be wrong for spec imports, so the html demo is only working because of a hack, it circumvents the one thing that browser imports can not do: resolving - at the expense of things like codesandbox or any other bundler that picks main, which are actually useful.

@looeee
Copy link
Collaborator

looeee commented Aug 14, 2019

Is anyone here using browser imports? And how?

I use them for development since it enables me to avoid waiting for the bundler every time I make a change.

I also use them for the examples on discoverthreejs.com (all of which run on codesandbox btw).

at the expense of things like codesandbox or any other bundler that picks main, which are actually useful.

You haven't demonstrated a bundler aside from codesandbox that does this.

Until you do that, I'm inclined to think that this is a codesandbox issue.

@drcmda
Copy link
Contributor Author

drcmda commented Aug 14, 2019

It's not a bundler issue. Three is published with 2 exports/separate namespaces:

  "main": "build/three.js",
  "module": "build/three.module.js",

Which one you use is up to you. In the examples it clings to a single namespace, in no possible scenario is this correct, even for those using draft imports. In essence, examples is broken for anyone that uses the cjs module export and that has to do with wrong module handling, not codesandbox.

Ives stated why he's doing it (in order to not transpile inside the browser) and there can be various reasons. If fixing this is too much to ask, i would suggest removing commonjs. If threejs doesn't support it, what's the point of it being there. If it's out, all will work again.

@gkjohnson
Copy link
Collaborator

@drcmda I think you're stance on this is clear -- I don't feel repeating your point is going to help move this issue anywhere and ignoring requests for more information, examples, or suggestions isn't making it easy to collaborate, either.

#16920 uses rollup to convert all the jsm files into the UMD equivelant in the /js folder which includes adding require('three') statements to import three.js. As it is I think merging that PR and updating the docs a little with where to import examples when using bundlers is enough to handle this for the moment.

@looeee
Copy link
Collaborator

looeee commented Aug 15, 2019

This is the 'standard' usage of three.js via npm, and it works fine:

  • install three from npm in a local dev environment
  • es6 import, say, renderer, camera, and controls from the npm package
  • Bundle with your preferred bundler: rollup, webpack, parcel.

What's happening with codesandbox is that they doing something nonstandard to save time. When they encounter this:

import * as THREE from "three";
import { OrbitControls } from "three/examples/jsm/controls/OrbitControls.js";

For the first import, they are ignoring the es6 'module' field and sticking with the 'main' cjs field, to save time.

But then they come to the OrbitControls import which specifies a single es6 file. There's no room for interpretation there and they have to use the es6 import... So now they are mixing cjs and es6 versions of three.js and you get two copies of the library in the bundle.

The issue here is that codesandbox is treating an es6 import as a cjs import. That's not correct and even though it works it has side effects like this.

However, I don't think this is a common issue, and it's not something we need to worry about. Codesandbox can fix it on their side if they choose, or they can keep doing this for faster builds at the expense of bigger bundles.

#16920 will also provide a solution for most people who want to use cjs imports but I don't think it will fix the codesandbox problem.

For everyone else doing things the standard way, they'll never come across this issue.

@drcmda
Copy link
Contributor Author

drcmda commented Aug 15, 2019

I don't feel repeating your point is going to help move this issue anywhere

I must say i feel kinda helpless. You are requesting more examples, but the root issue is ignored. Not a single person so far has acknowledged the fact that threejs offers two modules. What do you expect me to do other than trying to explain what is going on. As for a solution, it's pretty simple: either respect module resolution or do not expose two modules. If you must expose a commonJS module, but you do not want to support it, then it would be nice to document it or throw a console.warning.

What's happening with codesandbox is that they doing something nonstandard to save time.

And here we go again. No. They don't do anything non standard. They just pick the commonjs module, which threejs officially exports.

@looeee
Copy link
Collaborator

looeee commented Aug 15, 2019

You are requesting more examples, but the root issue is ignored

I'm requesting a clear demonstration of this happening anywhere except on codesandbox, a request that you have refused to acknowledge so far.

No. They don't do anything non standard. They just pick the commonjs module, which threejs officially exports.

This is a commonjs import, and should target 'main':

const THREE = require( 'three' );

This is an es6 import and should target 'module':

import * as THREE from 'three';

Doing otherwise, as codesandbox is doing, is not standard. In fact, apparently it's a violation of the es6 spec:

The TypeScript compiler at some point started allowing import * as foo from 'legacy-module-foo' to get the default import of a legacy module in certain circumstances. This is a violation of the ES6 specification (§15.2.1.16, “The value "*" indicates that the import request is for the target module’s namespace object.”).

@drcmda
Copy link
Contributor Author

drcmda commented Aug 15, 2019

I'm requesting any example of this happening anywhere except on codesandbox, a request that you have refused to acknowledge so far.

This is not the issue. The bundler has nothing whatsoever to do with anything.

If you fetch the commonjs module, with whatever system or means at your disposal then that creates a namespace. This namespace will always differ from the namespace that jsm/examples opens up, because it targets another module.

Here's another example in webpack/create-react-app: https://github.com/drcmda/three-cjs-bug/tree/master/src

It picks the commonjs export via

import * as THREE from "three/build/three"

And that's it. Two three bundles. Area lights gone.

I would be really glad if someone could at least acknowledge this. Why people and bundlers choose cjs is irrelevant. If we can agree that cjs and jsm are incompatible, which they are, all we need to do is decide if we fix it.

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 15, 2019

Well, what you are doing in your repository is obviously wrong. This is no demonstration of a bug. Yes, you can't use the UMD build three.js with JSM modules since they rely on three.module.js. However, this should be no problem if you have correct imports and a bundler works according to the standard.

I would appreciate if you recognize what @looeee mentioned in his previous post. It seems to me that codesandbox needs a fix. Not three.js.

@looeee
Copy link
Collaborator

looeee commented Aug 15, 2019

Your new example is incorrect since you are mixing cjs and es6 modules from a single npm package.

// This is a cjs module
import * as THREE from "three/build/three"
// This is an es6 module
import { OrbitControls } from "three/examples/jsm/controls/OrbitControls.js"

Just change that to import * as THREE from "three", as it should be, and it will work fine.

It seems like webpack is smart enough to handle cjs imports even though it's against the spec. Great. But if you really want to use the cjs version of three then you'll need to specifically request the cjs versions of the examples too, which you'll be able to do once #16920 lands.

To be clear, any bundler that encounters 'import * as X from 'package' in an npm package should use the 'module' field if present.

If they do otherwise, as codesandbox is doing, then they are incorrect and should expect problems like this one.

@gkjohnson
Copy link
Collaborator

gkjohnson commented Aug 15, 2019

@drcmda

I must say i feel kinda helpless. You are requesting more examples, but the root issue is ignored. Not a single person so far has acknowledged the fact that threejs offers two modules.

Sorry I didn't mean to come across that way -- I understand the issue you're describing and acknowledge that there can be an issue in some cases. It is possible to configure these bundling systems to pull from "main" instead of "module" but it seems like a nonstandard workflow.

I'll be quick to admit that the import / require ecosystem generally is a mess right now and I think we're just trying our best to make it work as cleanly for us as possible. Hopefully #16920 helps with that.

@looeee

It seems like webpack is smart enough to handle cjs imports even though it's against the spec. Great. But if you really want to use the cjs version of three then you'll need to specifically request the cjs versions of the examples too, which you'll be able to do once #16920 lands.

I'm sure this is what you meant but just to be clear the cjs examples should be able to be used universally when bundling three.js in an application with any of these tools. require('three') should resolve to the "module" field when importing three.js so it can be intermixed with import * as THREE from 'three'.

@looeee
Copy link
Collaborator

looeee commented Aug 15, 2019

I'm sure this is what you meant but just to be clear the cjs examples should be able to be used universally when bundling three.js in an application with any of these tools.

Well, yes, if you tell a tool to import a cjs file then it should do that. It's also fine for tools to import cjs using import * as - it seems like that's violation of the spec, but it's very convenient and common.

But if there is ambiguity, then a tool should always default to the es6 module when using import and always default to cjs when using require. For example, import * as THREE from 'three' is ambiguous since it's not telling the bundler exactly what file to use. But it's using import so it should always choose three.module.js if available. If you want cjs via an import statement then you should have to specify it using import * as THREE from "three/build/three".

If a bundler doesn't do this then you have a situation where this:

import * as THREE from "three";
import { OrbitControls } from "three/examples/jsm/controls/OrbitControls.js";

which is 100% correct es6 code, leads to wrong results. And that's just crazy and frustrating.

The only thing I'd say is 'wrong' here is when a person or bundler uses cjs and es6 from the same npm package. Of course that's still a relative statement and if it works in your project then great. But doing that is what's causing all the confusion here. Since codesandbox is not a private app then it seems like they should be more responsible and at least warn people they are importing cjs even though an es6 module is available.

@looeee
Copy link
Collaborator

looeee commented Aug 22, 2019

Closing this issue since it's a problem with codesandbox, not three.js

@looeee looeee closed this as completed Aug 22, 2019
@mrdoob
Copy link
Owner

mrdoob commented Sep 6, 2019

@drcmda Sorry for the delay. Just to make sure I understand...

Are you suggesting removing "main: "build/three.js" from package.json?

Or removing "module": "build/three.module.js" and setting "main": "build/three.module.js"?

@mrdoob
Copy link
Owner

mrdoob commented Sep 6, 2019

And, another noob question...

Say that three resolves to three/build/three.module.js and the dev writes this:

import * as THREE from "three";
import { OrbitControls } from "three/examples/jsm/controls/OrbitControls.js";

Are you saying that bundlers are not able to tell that the three/build/three.module.js that three resolves to and the ../../build/three.module.js that OrbitControls.js import from are the same file?

@drcmda
Copy link
Contributor Author

drcmda commented Sep 6, 2019

@mrdoob if commonjs is unsupported i would indeed remove it. there would only be one export: build/three.module.js and everything starts working again. i have since bumped into more and more of the same problem in similar circumstances. the dracoloader is broken too for instance - all due to module mismatch.

Are you saying that bundlers are not able to tell that the three/build/three.module.js that three resolves to and the ../../build/three.module.js that OrbitControls.js import from are the same file?

Exactly. If the bundler picks CJS, which it can, then you have two separate namespaces. There is no resolution happening if three/examples targets a specific module (build/three.module), while the bundle goes through standard package.json resolution and picks up commonjs, it has no chance of resolving the two properly. This is why i said the root issue is with three/examples. That's what resolution is there for, so that everything is on the same page.

The bundler isn't the issue. The issue is commonjs. If for whatever reason you ever pick commonjs, three and three/examples are not compatible any longer. This will hold true in any dev environment and all bundlers.

@looeee you keep saying this is a bundler issue, this is false. if three exposes two modules, bundlers are allowed to pick cjs. if that would violate a spec (which it doesn't), why do you expose cjs at all?

@mrdoob
Copy link
Owner

mrdoob commented Sep 6, 2019

Are you saying that bundlers are not able to tell that the three/build/three.module.js that three resolves to and the ../../build/three.module.js that OrbitControls.js import from are the same file?

Exactly. If the bundler picks CJS, which it can, then you have two separate namespaces. There is no resolution happening if three/examples targets a specific module (build/three.module), while the bundle goes through standard package.json resolution and picks up commonjs, it has no chance of resolving the two properly. This is why i said the root issue is with three/examples. That's what resolution is there for, so that everything is on the same page.

I think I was not clear. In the situation I'm proposing there is no commonjs export, only a single "main": "build/three.module.js".

Would a bundler not be able to tell that tell that the three/build/three.module.js that three resolves to and the ../../build/three.module.js that OrbitControls.js import from are the same file?

@drcmda
Copy link
Contributor Author

drcmda commented Sep 6, 2019

yes, that would work. 👍 now resolution and three/examples point to the same file. you could still distribute the commonjs file so people can use it, but if you don't declare it in package json, we're good.

well, not a 100%, because the commonjs module in build would still be incompatible, but cjs is dying and that's a good compromise i think.

@mrdoob
Copy link
Owner

mrdoob commented Sep 6, 2019

Okay.

And, in order to do that, is it just a matter of removing "module": "build/three.module.js" from package.json and changing "main": "build/three.js" to "main": "build/three.module.js"?

@mrdoob
Copy link
Owner

mrdoob commented Sep 6, 2019

Can we also remove jsnext:main?

@drcmda
Copy link
Contributor Author

drcmda commented Sep 6, 2019

i think at least module and main could point to the same thing. jsnext was obsolete, and if it's gone old bundlers would probably go towards main i guess. but any of these variations will work, main only, main and module or all three - pointing to build/three.module

@mrdoob
Copy link
Owner

mrdoob commented Sep 6, 2019

Okay. I'll try things for next release.

On npm-land, the only thing that will break is for people that rely on the commonjs build, right?

They'll have to update their code to require( 'three/build/three.js' )?

@mrdoob
Copy link
Owner

mrdoob commented Sep 6, 2019

@drcmda Many thanks for being so patient explaining all this 🙏

@drcmda
Copy link
Contributor Author

drcmda commented Sep 6, 2019

i think so, but it is more likely they were using "three" via resolution anyway. the only thing that could happen now is that their bundler doesn't understand the import statement, but i think there's no bundler that old in production left anymore ... webpack 1 or 2 maybe. in that case they import from 'build/three' like you said. thank you so much, too! :-)

@gkjohnson
Copy link
Collaborator

@mrdoob

And, in order to do that, is it just a matter of removing "module": "build/three.module.js" from package.json and changing "main": "build/three.js" to "main": "build/three.module.js"?

I'm not sure what your plan is with #16920 but if you're going to remove support for resolving "three" to the commonjs build then it may not make sense support require in the jsm -> js rollup conversion. It might make more sense to just produce an "iife" file, which would simplify the output a bit and prevent the js examples from breaking when calling require('three') in node.

@mrdoob
Copy link
Owner

mrdoob commented Sep 6, 2019

I'm not sure what your plan is with #16920 but if you're going to remove support for resolving "three" to the commonjs build then it may not make sense support require in the jsm -> js rollup conversion.

Yeah, that's why I was holding off that PR until things become more clear. I was even considering removing examples/js altogether, but I feel it's way too early for that?

I want to break as few things as possible while keeping the library approachable for people not using bundlers. And also painless for people using bundlers.

Is it okay to think that the bundler users that rely in commonjs shouldn't have a hard time updating to jsm?

@gkjohnson
Copy link
Collaborator

I want to break as few things as possible while keeping the library approachable for people not using bundlers. And also painless for people using bundlers.

I entirely agree and I would like to be able to propose more solutions but I'm having a hard time discussing this without any tangible, demonstrable examples of the problem with a bundler. It's not that I don't believe it can happen I just want to be more well informed. A small zip project demonstrating the loading problem when using DracoLoader, for example, would be fantastic. Without something like this I'm not sure how any of us can be expected to sympathetically discuss this.

Having said that I don't have a problem with changing "main" to point to three.module.js myself except it may complicate things when importing into node (see below). It's admittedly at odds with what the npm docs on package.json imply should be in that the field, though.

I was even considering removing examples/js altogether, but I feel it's way too early for that?

My opinion is that it's too early for that but if it's going to happen maybe it's worth embedding a deprecation warning log in the js example files for a few releases? I'd want a chance to clean up some of my github projects that depend on links to them, for example.

One other use case this impacts that likely isn't common is importing three.js and examples into node. I do this for small scripts, tests, benchmarks, etc. Changing "main" will mean require('three') will no longer work (which could be fine) and removing the js/examples will mean there's no process for importing example classes, loaders, etc into node without a build process, which to me would be a pain. It would be nice to have a reasonable way to support this (#16920 could have been an option). Right now those scripts will look something like this:

global.THREE = require( 'three' );
require( 'three/examples/js/controls/OrbitControls.js' );

console.log( THREE.OrbitControls ); // woo!

All that aside would we be able to get #16920 merged a little sooner if I change the output to just IIFE meaning no functionality would be added or removed compared to the current js example files? At least then the dev process for modules would cleaner while this is sorted out.

Is it okay to think that the bundler users that rely in commonjs shouldn't have a hard time updating to jsm?

In my experience most bundlers seem to let you use import and require interchangeably though maybe babel has to be involved in some cases?

@drcmda
Copy link
Contributor Author

drcmda commented Sep 7, 2019

A small zip project demonstrating the loading problem when using DracoLoader, for example, would be fantastic. Without something like this I'm not sure how any of us can be expected to sympathetically discuss this.

I think there have been enough demos though. All of the above highlight the problem. The sandbox picks cjs because it just works that way. The GH repo picks cjs because it can. The reason why anyone would want to use cjs doesn't matter. Now two three versions are loaded by the browser, which must naturally clash because three mutates its own namespace.

IMO we don't have to test that against each and every thing, dracoloader etc, b/c when three adds any new information to its module-namespace it just won't be accessible in the cjs namespace. This is a clear violation of npm/node resolution, and i'm quite sure that among the millions of libs on npm three is probably the only one doing this.

But thankfully cjs is an obsolete concept, so it's not that hard to fix. Node has moved on and understands imports, all bundlers do, require could be marked obsolete here. And as we see from all these examples, in a way it already is. Removing it at least from package.json would be a good compromise, as it doesn't upset anything else (one could hope).

The real fix

... would be to change everything in three/examples to import {...} from 'three'. This would break the html demos. But as i have said before, you're trying to cater to a standard that does not exist. Standard modules lack resolution, so they are a 100% reliant on build tools.

We are hacking around that by priming everything to a fixed module, thereby throwing npm under the bus. But standard modules are still useless, because you would not be able to use this in the real world. Every 3rd party lib you want to use has to be transpiled (resolved). Just like you had to transpile jsm to "three.module" to make it work.

I think the es spec should not be used until it's completely ratified and esp until they have solved the many issues that still plague it.

@gkjohnson
Copy link
Collaborator

gkjohnson commented Sep 7, 2019

Node has moved on and understands imports

I don't think this is a fair claim quite yet. Modules have been behind an experimental flag for years, have recently had their implementation changed and might be released for real in October. If the node.js case is considered worth supporting at all then I think removing the cjs version outright might be a bit hasty. But it sounds like it's coming sooner than I thought.

I think there have been enough demos though. All of the above highlight the problem. The sandbox picks cjs because it just works that way. The GH repo picks cjs because it can.

I agree that codesandbox illustrates the issue as we've discussed it but your github demo does not. Here's why.

import * as THREE from "three/build/three"

This line subverts modules resolution which is the core of this discussion and therefore does not illustrate the problem. None of the three.js documention or standard use of the library support or advocate for the use of this import statement. Likewise someone could download their own version of three.js and import that as well as an example from this project and it would break in the same way.

None of the proposed solutions solve this case. That repo would break in the same way even after changing the example modules to resolve from three because the bundlers will resolve three/build/three to a different file from three. The same thing will happen if you change the main field in package.json. Do you understand my confusion? You've presented something as a repro case but none of the solutions you're happy with actually solve it so it cannot really be considered a repo case of the issue.

I'm looking for a repro of a bundler setup that illustrates importing from three and an example file but results in two versions of three.js included in the page. It was claimed previously that many projects may be inadvertently including two versions of three.js in the page if they use examples. How can this happen on accident? What would the bundler config look like?

If we're actually just talking about the codesandbox case then that's fine and @mrdoob's proposed solution will solve it just fine.

@drcmda
Copy link
Contributor Author

drcmda commented Sep 7, 2019

I don't think this is a fair claim quite yet. Modules have been behind an experimental flag

I am not talking about a spec. The world uses node resolution, and everything has moved on from cjs, all bundlers and node itself. In other words, they understand the import syntax.

This line subverts modules resolution

Yes, it demonstrates what three itself is doing.

None of the proposed solutions solve this case. That repo would break

I know. Some people still didn't understand namespaces. I am showing that one can't mutate one namespace and expect the other to work.

And yes, cjs is and always will be incompatible (unless three/exports is fixed). But at least standard package.json node resolution would always lead to three/build/three.module which solves it because tools understand "import". Codesandbox would happily use the es-module.

In essence

let's please just remove the fields in package.json that lead to build/three. Perhaps a warning somewhere in the docs that three considers cjs obsolete since it never had add-on support. That it worked in some cases was sheer accident.

And in the future

i would really suggest fixing examples. Relying on a draft spec just to have some htmls appear to run in the browser "without build tools" is a bad idea. It doesn't need build tools because build tools were used to skip module resolution.

@gkjohnson
Copy link
Collaborator

gkjohnson commented Sep 7, 2019

Okay this sounds more like a philosophical discussion on the right / clean way to host and develop a javascript codebase independent of the fact that the current practice here is not breaking any real world projects with bundlers (aside from codesandbox). Am I understanding right?

@adrian-delgado
Copy link

If each jsm example would be its own package, then they would import from 'three' directly and treat three as a peerDependency. If they are in the same package as three (but as subfolders), then it just happens that they can import either from 'three' or from 'path/to/three.module.js' (but not in all cases, like the codesandbox example).

This does seem to be more related to how es imports are meant to be treated. If 'some-package' and 'some-package/sub-path' are treated as independent modules, then the real fix proposed by @drcmda should be considered.

@drcmda
Copy link
Contributor Author

drcmda commented Sep 13, 2019

the current practice here is not breaking any real world projects with bundlers (aside from codesandbox)

it breaks commonjs, not codesandbox. codesandbox is broken because it uses commonjs. every project in the real world that is commonjs based cannot use three/examples. that is, it can, but it's sheer accident if something works.

If each jsm example would be its own package, then they would import from 'three' directly and treat three as a peerDependency. If they are in the same package as three (but as subfolders), then it just happens that they can import either from 'three' or from 'path/to/three.module.js' (but not in all cases, like the codesandbox example).

im fine with either. but the moment three exposes two namespaces it won't work. three has to decide what it wants. it could fix examples, or remove one namespace from package json.

@mrdoob
Copy link
Owner

mrdoob commented Sep 14, 2019

Moving the conversation to #17482. This thread got way too confusing.

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

No branches or pull requests

7 participants