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

Support esm on node with conditional exports #18498

Closed

Conversation

sebamarynissen
Copy link

What changes are included in this PR?

Since 13.6 node is able to load esm without the --experimental-modules flag, and since 13.7 node supports conditional exports unflagged as well.

I modified the build config to support importing three.js from node as an ES module. As such code like

import { BufferGeometry } from 'three';

becomes possible on node. Without this only

import three from 'three';

is possible on node, which is a pity if you've written all your code to use named exports.

This approach implies that an additional build file is created called three.mjs. In theory we could use the three.module.js file as well, but there are some problems with this. Because there's no package.json with "type": "module" in the build folder, node will interpret three.module.js as a cjs module. There are a few imaginable solutions for this:

  • Rename three.module.js to three.module.mjs and change "module": "build/three.module.js" to "module": "build/three.module.mjs". This breaks all code explicitly depending on the file though.
  • Add a package.json file with { "type": "module" } in the build folder. This however would cause node to interpret all .js files in /build as an ES module, which has the same drawbacks if someone on node explicitly depends on const three = require('three/build/three.js');

Given that both solutions aren't ideal, I went with the option to simply create an entire new build, which is a duplicate though of three.module.js.

Why does one need to import three.js on node?

Obviously three.js is meant to be run in a browser, but it may be useful to be able to run it on node as well for the sake of unit testing. As such one can run unit tests without a transpilation step, especially since mocha experimentally supports esm as well.

@sebamarynissen sebamarynissen requested a review from mrdoob January 28, 2020 08:44
@sebamarynissen
Copy link
Author

sebamarynissen commented Jan 28, 2020

I've found that there's actually another possibility as well. If we add a package.json file inside /src with

{
  "type": "module"
}

then we could change the main package.json to

{
  "exports": {
    "require": "./build/three.js",
    "import": "./src/Three.js"
  }
}

I have tested this and it works, but has a horrible loading time (~300ms), whereas the prebuilt version loads pretty much instantaneously (~30ms).

@gkjohnson
Copy link
Collaborator

I feel this is a bit too redundant and confusing. With node now properly supporting modules I think we can look into deprecating the UMD / require build -- rollup, parcel, webpack, browsers, and some browser-based fiddle / pen editors support modules now. Adding "type": "module" to the package.json and setting "main" to be build/three.module.js would address the concerns in this PR and allow for closing #17482.

Perhaps the UMD build variant can remain available for a bit and be removed alongside the examples/js files?

@donmccurdy
Copy link
Collaborator

I guess this is implied by the PR, but why does node.js not recognize "module": "build/three.module.js", in the existing package.json file? It needs type: module and an .mjs extension, too? 😕

@sebamarynissen
Copy link
Author

sebamarynissen commented Jan 28, 2020

This would indeed be the most elegant solution, but it is of course a breaking change: node versions that do not support es modules will no longer be able to

const three = require('three');

My approach would be to at least have the UMD build still available somewhere. In that case it needs to have a .cjs extension, or in a separate folder with its own { "type": "commonjs" } so that it can still be reached using

const three = require('three/build/three.cjs');

@donmccurdy This is how esm works on Node: It can't use the "module" field because it was introduced by bundlers which generally do not require extensions to be specified - and hence a lot of packages don't - but node does require extensions to be specified in esm. Therefore they introduced the "type": "module" approach.

@yushijinhun
Copy link
Contributor

yushijinhun commented Jan 29, 2020

I am opposed to single .mjs file. It reduces the loading time, but also makes it difficult for bundlers to perform tree-shaking (see #16059 (comment)).

Here are the factors you have to take into consideration when solving this issue:

My idea to solve this problem

  • Save unbundled build outputs to libs/
    • glsl and gl-constants build optimization are performed
    • they are not bundled into a single file, and file hierarchy is kept
  • Rename three.module.js to three.mjs
  • Point main to build/three.mjs
  • Point module to libs/Three.js
  • Drop the UMD build

Since node recognizes main while bundlers prefer module, node will use the single build/three.mjs file and bundlers will use the tree-shakable libs/Three.js.

But this solution is too aggressive and breaks the backward compatibility greatly, I'm not sure whether it can be accepted.

@donmccurdy
Copy link
Collaborator

Thanks for the explanation, @sebamarynissen.

@gkjohnson @yushijinhun I'm having a hard time justifying giving up CommonJS/UMD support on the main entrypoint, breaking older web bundlers like Browserify in order to make Node.js work better. While I would like to support Node.js as best we can, a lot of users still use CommonJS for web bundling, and (IMO) that is probably a higher priority at this point.

We'll probably eventually have to drop the examples/js folder, as you mention, but justification for that seems much clearer than breaking main.

@sebamarynissen
Copy link
Author

sebamarynissen commented Jan 29, 2020

@yushijinhun Is there any reason you don't want to use conditional exports here? Conditional exports will behave the same way:

  • Bundlers will still prefer module. Hence we can point this to libs/Three.js as you mention to support tree shaking.
  • We don't touch the main field so Node versions not supporting conditional exports will just load the UMD build.
  • Node versions that do support conditional exports will load either the esm or UMD build, depending on whether three was required or imported.

The only drawback I see with this approach is that conditional exports is currently still experimental, so on Node 13.7 you will get a warning of it, even if you just

const three = require('three');

A future problem with this approach might be that if bundlers would ever take into account conditional exports as well and have it take precedence over the module field, then we lose the tree-shaking again.

@yushijinhun
Copy link
Contributor

A future problem with this approach might be that if bundlers would ever take into account conditional exports as well and have it take precedence over the module field, then we lose the three-shaking again.

@sebamarynissen As far as I know, no bundler is going to support conditional exports. So currently, we are safe.

Your idea does have a better backward compatibility, but as is mentioned in Node.js documentation, dual CommonJS/ES Module package is really a "hazard". Dropping UMD support might be aggressive, but it reduces the complexity and avoids some potential problems. Anyway, it's up to the project director to decide which approach to take.

@MylesBorins
Copy link

MylesBorins commented Jan 29, 2020

One issue with the current implementation is that it potentially creates two separate cache entries (esm + cjs). If three.js is imported and required you will create two instances which can cause super hard to debug errors.

More info about this hazard is in the NodeJS docs

https://nodejs.org/dist/latest-v13.x/docs/api/esm.html#esm_dual_package_hazard

The "wrapper" pattern is one that you could use in three that could avoid the roll-up step

https://nodejs.org/dist/latest-v13.x/docs/api/esm.html#esm_approach_1_use_an_es_module_wrapper

@yushijinhun
Copy link
Contributor

About examples (#17482 (comment))

Another problem is that currently we cannot use bare module specifiers (import "three";) in examples/, because they are not supported by browsers. We will have to use ../../../build/three.module.js or ../../../libs/Three.js.

import {
EventDispatcher,
MOUSE,
Quaternion,
Spherical,
TOUCH,
Vector2,
Vector3
} from "../../../build/three.module.js";

Using import-maps may solve this problem.

@sebamarynissen
Copy link
Author

@MylesBorins Perhaps that I can elaborate a bit on why I went with the ES module wrapper approach in chaijs/chai#1317, but not here. The main reason is that chai is not yet written using esm, but three.js is. As such it felt "natural" to not depend on the cjs build after all. But as you say, this indeed introduces the "dual package" hazard.

Also, Chai only has a limited amount of what could be considered as named exports. Three.js has quite a lot of them which may make it harder to keep the esm wrapper file for node in sync with what three.js exports.

Perhaps I can clarify a bit what this wrapper file approach would look like for three.js for people not familiar with it:

// three.mjs
import three from '.';

const { BufferGeometry, Vector3, And, Everything, Three, Exports } = three;
export { BufferGeometry, Vector3, And, Everything, Three, Exports };

// package.json
{
  "exports": {
    "require": "./build/three.js",
    "import": "./three.mjs"
  }
}

@MylesBorins
Copy link

One thing to mention, which was figured out in today's Node.js modules call. With the following pattern there will be no warning when requireing the package (there will still be a warning for ESM)

{
  "exports": {
    "import": "./three.mjs",
    "default": "./build/three.js",
  }
}

This is because the default export is not part of the conditional exports feature and does not trigger the warning. With that being said I am unsure that is an accurate representation of what would theoretically be the default.

@sebamarynissen
Copy link
Author

Thanks, I will publish a new version tomorrow. A warning for ESM is fine because ESM itself is still experimental anyway. The warning when requiring was a bit cumbersome though. Glad to hear it can be solved!

@drcmda
Copy link
Contributor

drcmda commented Feb 11, 2020

One issue with the current implementation is that it potentially creates two separate cache entries (esm + cjs). If three.js is imported and required you will create two instances which can cause super hard to debug errors.

Another problem is that currently we cannot use bare module specifiers (import "three";) in examples/, because they are not supported by browsers. We will have to use ../../../build/three.module.js or ../../../libs/Three.js.b

Currently, to fix it, i'm doing:

import * as THREECJS from 'three'
import * as THREE from 'three/build/three.module'
Object.assign(THREE, THREECJS)

This is the only way to run addons like ReactAreaLights in cjs, many others are broken as well (basically every add-on that relies on namespace mutation, like extending shaderlib etc). If we could remove cjs completely (or at least remove the entry in package.json) that would be such a relief. It would be no problem at all when examples import from ../../../build/three.module.js

@donmccurdy
Copy link
Collaborator

Another problem is that currently we cannot use bare module specifiers (import "three";) in examples/

Tested this out recently, and we could get around it by including an import map polyfill in each example page: https://github.com/KhronosGroup/KTX-Software/pull/172/files. I don't know when/if import maps will come to browsers natively, though.

@cazen5050
Copy link

in node , i want to use gltfloader.js :
import {GLTFLoader} from 'three/examples/jsm/loaders/GLTFLoader.js'

but error :
node_modules/three/build/three.module.js:39868
const req = new Request( url, {
^
ReferenceError: Request is not defined

@sebamarynissen
Copy link
Author

@cazen5050 Not sure if this really belongs here, but the reason is that Request is a browser API and hence not available in Node. You can polyfill it like this (though I did not test it).

import fetch, { Headers, Request, Response } from 'node-fetch';

if (!globalThis.fetch) {
  Object.assign(globalThis, {
    fetch,
    Headers,
    Request,
    Response,
  });
}

Also, apparently Node 18 will include this natively (see also https://www.stefanjudis.com/notes/global-fetch-landed-in-node-18/).

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

Successfully merging this pull request may close these issues.

8 participants