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

Add .cjs build output needed for commonjs environments #23325

Merged
merged 2 commits into from
Jan 25, 2022

Conversation

marcofugaro
Copy link
Contributor

@marcofugaro marcofugaro commented Jan 25, 2022

Related issue: Fixes #23324

Description

According to the docs one more thing is needed, an esm module must provide a .cjs file for commonjs environment.

Added a short script that copies over build/three.js to build/three.cjs.

To test this, I've created a simple commonjs test environment:

  1. Download this zip: three-commonjs-test.zip
  2. Extract folder and cd into it
  3. Make sure you have Node 16 or greater, so it can read the exports field
  4. Run npm i
  5. Run npm start
  6. No error should appear and 137dev should be the console output

@Mugen87 Mugen87 added this to the r137 milestone Jan 25, 2022
@CodyJasonBennett
Copy link
Contributor

Looks good. The main/module field resolution also works across Node 14-16 with the usual suspects of Node tools like Next and Webpack (via CRA).

It should be noted that commonjs users that use a bundler will have to also resolve files with a cjs extension if not already. Probably good to note this in the release notes alongside the new three/addons/* alias?

@marcofugaro
Copy link
Contributor Author

marcofugaro commented Jan 25, 2022

@CodyJasonBennett thanks for testing!

It should be noted that commonjs users that use a bundler will have to also resolve files with a cjs extension if not already.

I tested this, all modern bundlers (even with commonjs) work this PR by default, no need for the user to do anything.

alongside the new three/addons/* alias?

It's not there yet, it's just a rumor, mrdoob decides when he wants to go through with it 😛

@mrdoob mrdoob merged commit ea6348d into mrdoob:dev Jan 25, 2022
@mrdoob
Copy link
Owner

mrdoob commented Jan 25, 2022

Thanks!

@mrdoob
Copy link
Owner

mrdoob commented Jan 25, 2022

It's crazy how many dependencies cpy-cli brings though...

This is slowly becoming a quite maintenance burden. Just yesterday I had to check when puppeteer was going to update their npm module because the version of node-fetch they were using had a vulnerability.

The more dependencies, the more often we'll have to run npm audit fix or go to respective github repos to ask for updates about their npm module.

In fact, just by adding cpy-cli we now have 5 vulnerabilities:

Screen Shot 2022-01-25 at 9 15 47 AM

@mrdoob
Copy link
Owner

mrdoob commented Jan 25, 2022

I've removed cpy-cli and generated the three.cjs file from rollup.config.js instead. 1625f32

@marcofugaro
Copy link
Contributor Author

Thanks @mrdoob! It was the first library I came across hahah

Even better you can set format: 'cjs' in rollup 🙂

@mrdoob
Copy link
Owner

mrdoob commented Jan 25, 2022

Like this?

			{
				format: 'umd',
				name: 'THREE',
				file: 'build/three.js',
				indent: '\t'
			},
			{
				format: 'cjs',
				name: 'THREE',
				file: 'build/three.cjs',
				indent: '\t'
			}

@marcofugaro
Copy link
Contributor Author

Yup!

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.

adding the type key "module" on the root will make threejs ESM-only
4 participants