-
-
Notifications
You must be signed in to change notification settings - Fork 35.5k
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
Tests: use ES modules #23315
Tests: use ES modules #23315
Conversation
Nice! Do we still need the files section? Lines 12 to 23 in be9d017
|
Yes 🙂 that field contains the files that are downloaded when a user installs three from npm. |
"import": "./build/three.module.js", | ||
"require": "./build/three.js" | ||
}, | ||
"./examples/jsm/*": "./examples/jsm/*.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would happen if we renamed this? Say...
"./addons/*": "./examples/jsm/*.js",
Would people be able to this?
import { OrbitControls } from 'three/addons/controls/OrbitControls.js'
And, would the old version stil work?
import { OrbitControls } from 'three/examples/jsm/controls/OrbitControls.js'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently yes, you can do that, from the docs:
And, would the old version stil work?
Only if you have both of the import aliases in the exports
field:
"./addons/*": "./examples/jsm/*.js",
"./examples/jsm/*": "./examples/jsm/*.js",
Otherwise node (or any bundler) throws an error if you're trying to import something that is not in the export
fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent! That makes thing easy.
What do you think about the name addons
? Do you think @drcmda will like it? 🤓
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hahaha I don't mind the name!
I even made a three-addons
package back when examples/jsm
wasn't a thing 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I'll keep it in mind 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mrdoob i do like it 😁
Thanks! |
Related issue: Fixes #23304
Description
While we're at it, let's update the tests to use es modules introduced in Node 16.
No need to use rollup in tests anymore.
Following the Node.js docs, the new package.json looks like this:
Note that I had to include the
src
folder in the exports field as well, since the tests are importing from thesrc
folder.I also added the
.js
extension to imports since in esmodules (both web and node), the extension has to be explicit.You can test this PR by:
npm i --prefix test
npm run test-unit
npm run test-unit-examples
To test the browser version
npm i --prefix test
npm start
Note that the web tests work only in Chrome/Edge because of import maps. The import maps polyfill doesn't work with Qunit because it's syncronous.
/cc @takahirox