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

module: clarify CJS global-like variables not defined error message #37852

Merged
merged 1 commit into from Apr 13, 2021

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Mar 21, 2021

Error message on Node.js v15.x:

ReferenceError: require is not defined
      at data:text/javascript,require;:1:1
      at ModuleJob.run (node:internal/modules/esm/module_job:154:23)
      at async Loader.import (node:internal/modules/esm/loader:166:24)
      at async importModuleDynamicallyWrapper (node:internal/vm/module:437:15)
      at async waitForActual (node:assert:736:5)
      at async Function.rejects (node:assert:845:25)

Error message with this PR:

ReferenceError: require is not defined in ES module scope, you can use import instead
      at data:text/javascript,require;:1:1
      at ModuleJob.run (node:internal/modules/esm/module_job:155:25)
      at async Loader.import (node:internal/modules/esm/loader:166:24)
      at async importModuleDynamicallyWrapper (node:internal/vm/module:437:15)
      at async waitForActual (node:assert:736:5)
      at async Function.rejects (node:assert:845:25)

@nodejs/modules I'm not sure we should keep the .cjs advice because it doesn't always apply – sometimes there are no files (E.G.: data: URL), and this behaviour could be overridden by a custom loader. On the other hand, for a user stumbling over this issue, the suggestion could be helpful. wdyt?

Fixes: #33741

In the issue, it's also been discussed to add a message explaining why the file was parsed as ESM rather than CJS, but I couldn't find how to do that.
EDIT: When the module is a file with .js extension, the following error message is shown:

ReferenceError: require is not defined in ES module scope, you can use import instead
This file is being treated as an ES module because it has a '.js' file extension and '/…/package.json' contains "type": "module". To treat it as a CommonJS script, rename it to use the '.cjs' file extension.

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Mar 21, 2021
@aduh95 aduh95 added the esm Issues and PRs related to the ECMAScript Modules implementation. label Mar 21, 2021
@aduh95 aduh95 changed the title module: clarify require not defined error message module: clarify CJS global-like variables not defined error message Mar 21, 2021
@aduh95
Copy link
Contributor Author

aduh95 commented Mar 21, 2021

I've added detection of other global-like CJS variables (exports, __filename, etc.).

Comment on lines 185 to 189
'. It seems you are trying to load a file using `.js` extension ' +
'inside a folder containing a `package.json` that includes ' +
'`"type": "module"`; you need to use the `.cjs` extension to ' +
'load the file as a CommonJS module, or add a `package.json` ' +
'file with ``"type": "commonjs"`.';
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 way too long and reminds me of clippy. Something more short and to the point would be better:

Suggested change
'. It seems you are trying to load a file using `.js` extension ' +
'inside a folder containing a `package.json` that includes ' +
'`"type": "module"`; you need to use the `.cjs` extension to ' +
'load the file as a CommonJS module, or add a `package.json` ' +
'file with ``"type": "commonjs"`.';
'CommonJS files can be loaded using the package.json type option or .cjs'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that it's not much longer than the error message when using named import with CJS:

e.message = `Named export '${name}' not found. The requested module` +
` '${childSpecifier}' is a CommonJS module, which may not support` +
' all module.exports as named exports.\nCommonJS modules can ' +
'always be imported via the default export, for example using:' +
`\n\nimport pkg from '${childSpecifier}';\n${
destructuringAssignment ?
`const ${destructuringAssignment} = pkg;\n` : ''}`;

In the linked issue, we have a user reporting they got such error without realising they were under a "module" package scope, having a very explicit error message would help I think. (and also everyone loved Clippy anyway, right? right? 🙃). Anyway, if the consensus is to have a shorter message, I'll take your suggestion.

Copy link
Member

Choose a reason for hiding this comment

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

I was just about to say, we should make this similar to the “named export” error message, which includes the path to the controlling package.json file and things like that. An error message that includes the context to explain what’s going on seems worth the added length. Something like:

your-file-here.js cannot be parsed as an ES module, but it is being treated as an ES module
because it has a .js extension and /Users/devsnek/projects/foo/package.json contains
"type": "module". To treat it as a CommonJS file, rename it to your-file-here.cjs.

I’m not sure we want to recommend changing the relevant "type" value, since that would likely affect other files that might not be CommonJS. We could also do without the second sentence in my example above, though then we’re leaving the user to Google the error message to figure out what to do to fix this. (The other potential solution that comes to mind is to refactor the offending file into ESM, but that seems a bit much for an error message to suggest.)

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, we could link to the docs rather than including the second sentence, and the docs are able to be much more precise without having to be quite so brief.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we do link to our docs in any error message so far, maybe there's a good reason for not doing it? If there's none, that seems ideal indeed.

if (e.name === 'ReferenceError' &&
StringPrototypeEndsWith(e.message, ' is not defined') &&
isCommonJSGlobalLikeNotDefinedError(e.message)) {
e.message += ' in ES module scope';
Copy link
Member

Choose a reason for hiding this comment

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

more than one of these if statements can be true at the same time. we should ensure every combination is a valid sentence or sentences.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well I think it is already, isn't it? Do you want me to add a test with all conditions being true at the same time?

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of in ES module scope perhaps we can just say in the ES module ${url}.

Do you want me to add a test with all conditions being true at the same time?

It sounds to me like that is the request here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can just say in the ES module ${url}.

The url is already provided in the first line of the stack trace, is it useful to repeat it here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good poin - if we definitely have an error frame for all of these then we should work to that.

lib/internal/modules/esm/module_job.js Outdated Show resolved Hide resolved
@aduh95
Copy link
Contributor Author

aduh95 commented Mar 25, 2021

@devsnek Are you still -1 on the latest version of this PR?

Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

This is a great one, thank you!

if (e.name === 'ReferenceError' &&
StringPrototypeEndsWith(e.message, ' is not defined') &&
isCommonJSGlobalLikeNotDefinedError(e.message)) {
e.message += ' in ES module scope';
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of in ES module scope perhaps we can just say in the ES module ${url}.

Do you want me to add a test with all conditions being true at the same time?

It sounds to me like that is the request here.

e.message += ' in ES module scope';

if (StringPrototypeStartsWith(e.message, 'require ')) {
e.message += ', you can use import instead';
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps - Use "import" instead or

Copy link
Contributor Author

@aduh95 aduh95 Mar 25, 2021

Choose a reason for hiding this comment

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

Is your comment incomplete or? 😅
For reference, the e.message at this point is ReferenceError: require is not defined in ES module scope.

Copy link
Contributor

Choose a reason for hiding this comment

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

Was working to ReferenceError: require is not defined in the ES module ${url} per the previous comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

The or was supposed to compose with the .cjs extension suggestion, but we could keep that at the end too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see! Note that the second part is printed out only if the module is .js, we need to take into account the case where the module is using a different extension or is a data: URL.

lib/internal/modules/esm/module_job.js Outdated Show resolved Hide resolved
@devsnek
Copy link
Member

devsnek commented Mar 25, 2021

I came across something really annoying yesterday where I had to use e.message.split('\n')[0] to achieve proper presentation because node was inserting a bunch of garbage into the message. I think we should hold off on this or put the text somewhere else. Maybe we can attach a detail property or something?

@guybedford
Copy link
Contributor

@devsnek that's a good point, but I don't think now is the time rework long error messages in Node.js that provide context. Rather that should ideally be a separate PR process.

@devsnek
Copy link
Member

devsnek commented Mar 25, 2021

@guybedford sure, I'm saying at the very least, hold off on landing this until we figure that out. I'm personally willing to put in that effort (this weekend perhaps).

@guybedford
Copy link
Contributor

@devsnek that could work, can we at least set a rough time frame on such a hold to ensure this doesn't go stale?

@ljharb
Copy link
Member

ljharb commented Mar 25, 2021

We could use the cause property (without waiting for the constructors to support it) since that’ll be what’s standard

@aduh95
Copy link
Contributor Author

aduh95 commented Apr 2, 2021

@devsnek would it be OK to land this now?

I think we should hold off on this or put the text somewhere else. Maybe we can attach a detail property or something?

BTW if you want/need help on that, please reach out. I agree with Jordan, adding a cause property seems like a plan.

@aduh95 aduh95 dismissed devsnek’s stale review April 11, 2021 17:41

Dismissing due to unresponsiveness.

@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 11, 2021
@aduh95
Copy link
Contributor Author

aduh95 commented Apr 11, 2021

I'm planning to land this in a few days if no one objects.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Fixes: nodejs#33741

PR-URL: nodejs#37852
Reviewed-By: Guy Bedford <guybedford@gmail.com>
@aduh95
Copy link
Contributor Author

aduh95 commented Apr 13, 2021

Landed in 7b2bad4

@aduh95 aduh95 merged commit 7b2bad4 into nodejs:master Apr 13, 2021
@aduh95 aduh95 deleted the require-not-defined-error branch April 13, 2021 17:07
BethGriggs pushed a commit that referenced this pull request Apr 15, 2021
Fixes: #33741

PR-URL: #37852
Reviewed-By: Guy Bedford <guybedford@gmail.com>
targos pushed a commit that referenced this pull request May 30, 2021
Fixes: #33741

PR-URL: #37852
Reviewed-By: Guy Bedford <guybedford@gmail.com>
targos pushed a commit that referenced this pull request Jun 5, 2021
Fixes: #33741

PR-URL: #37852
Reviewed-By: Guy Bedford <guybedford@gmail.com>
targos pushed a commit that referenced this pull request Jun 11, 2021
Fixes: #33741

PR-URL: #37852
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ReferenceError: require is not defined
7 participants