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

Implement ERR_REQUIRE_ESM when a file is require()d which should be loaded as ESM #1031

Merged
merged 18 commits into from May 20, 2020

Conversation

cspotcode
Copy link
Collaborator

Node's built-in require.extensions['.js'] will detect if a file should be loaded as ESM instead of CommonJS, according to the file's associated package.json. If it should be loaded as ESM, node throws an error. This change makes our require.extensions throw the same error.

Mocha, for example, is relying on this error to detect when files are ESM and when they're CommonJS.
https://github.com/mochajs/mocha/blob/master/lib/esm-utils.js#L10-L22

This change detects when we're on a version of node that supports ESM, so it knows if it needs to (possibly) throw this error. If so, it runs the same code as node to find the relevant package.json and see if the file should be treated as ESM. If so, it throws an error that matches node's built-in error. I did this by copy-pasting the relevant bits of node's source code.

Things I don't really love about this:

  • ESM is imposing changes on our CommonJS hooks
  • copy-pasting more code out of node's source
  • feature-detection method. Not sure I am using the best approach.
    • Alternative: KISS and check if node version is >=12 (even faster)
    • Alternative: we can bundle a subdirectory like the following, then attempt to require() the .js file and see if we get the error. (slower but is proper "feature detection")
./esm-detection
  ./package.json -> {"type": "module"}
  ./index.js -> empty file

Looking at how a few node versions behave:

node 12 always parses import() syntax, doesn't support native ESM without the --experimental-modules, but always throws the error if you try to require() a file that should be loaded as ESM.

node 10 & 11 always parses import() syntax, only loads ESM with --experimental-modules, and never honors the package.json "type" field.

@coveralls
Copy link

coveralls commented May 9, 2020

Coverage Status

Coverage increased (+1.1%) to 82.847% when pulling 1a21a7d on ab/add-err-require-esm-error into 196aafb on master.

@cspotcode
Copy link
Collaborator Author

This is technically a breaking change. Where previously we would attempt to execute .ts files as CJS even if the engine would otherwise honor package.json "type": "module", now we will throw an error.

I can implement this change conditionally for now, so it's only turned on if our ESM loader is used.

Node 12's behavior is weirder than I thought depending on the minor version. Based on the behavior, I think checking process.versions.node >=12 is best. Other forms of feature detection will log warnings or give false negatives.

node 12.15
require()s ESM package executes as CJS and logs warning about not supporting ESM (we want to avoid the warning)

node 12.16 throws an ERR_REQUIRE_ESM
yet package.json "exports" are not implemented

@cspotcode cspotcode marked this pull request as draft May 9, 2020 23:04
@cspotcode cspotcode marked this pull request as ready for review May 16, 2020 13:07
const err = new Error(getMessage(filename, parentPath, packageJsonPath))
err.name = `Error [${ code }]`
err.stack
Object.defineProperty(err, 'name', {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty funky, might want to add a comment on why you do this since the first look makes it seem this line would erase err.name = above.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -0,0 +1,118 @@
// copied from https://github.com/nodejs/node/blob/master/lib/internal/modules/cjs/loader.js
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this whole file copied or which parts should be reviewed? Curious on things like we read JSON ourselves instead of relying on the require cache.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, this is actually coming from several files in node's source. I added comments to each item in this file with a permalink to the span of node's source code on github.

@cspotcode
Copy link
Collaborator Author

@blakeembrey

Thanks for the review. I added comments explaining what's going on in dist-raw/node-cjs-loader-utils.js.

Re using the require cache to read package.json files:
I don't have a strong opinion either way. Admittedly I was a tiny bit annoyed at having to duplicate more of node's source-code, so I wanted to copy-paste with as few modifications as possible, to make sure our stuff behaves as close as possible to node.

Would you prefer I tweak that code to use require()? On second thought, seems like that would work perfectly fine.

@cspotcode cspotcode force-pushed the ab/add-err-require-esm-error branch from f3e3f18 to 6b15230 Compare May 18, 2020 06:00
@codecov
Copy link

codecov bot commented May 18, 2020

Codecov Report

Merging #1031 into master will increase coverage by 0.11%.
The diff coverage is 79.31%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1031      +/-   ##
==========================================
+ Coverage   73.57%   73.68%   +0.11%     
==========================================
  Files           4        6       +2     
  Lines         507      608     +101     
  Branches      130      142      +12     
==========================================
+ Hits          373      448      +75     
- Misses         83      101      +18     
- Partials       51       59       +8     
Flag Coverage Δ
#node_10 70.89% <16.66%> (-0.70%) ⬇️
#node_12 ?
#node_12_15 71.28% <16.66%> (?)
#node_12_16 71.28% <16.66%> (?)
#node_13 73.51% <79.31%> (+0.14%) ⬆️
#node_14 73.51% <79.31%> (+0.14%) ⬆️
#typescript_2_7 73.19% <79.31%> (+0.21%) ⬆️
#typescript_latest 72.36% <79.31%> (+0.17%) ⬆️
#typescript_next 72.20% <79.31%> (+0.21%) ⬆️
#ubuntu 73.35% <79.31%> (-0.02%) ⬇️
#windows 73.51% <79.31%> (+0.14%) ⬆️
Impacted Files Coverage Δ
src/index.ts 75.00% <66.66%> (+0.07%) ⬆️
dist-raw/node-cjs-loader-utils.js 80.39% <80.39%> (ø)
src/esm.ts 66.66% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 196aafb...1a21a7d. Read the comment docs.

@cspotcode cspotcode merged commit 3978f32 into master May 20, 2020
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.

None yet

3 participants