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

aduh95 · 2021-03-21T14:50:44Z

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.

lib/internal/modules/esm/module_job.js

aduh95 · 2021-03-21T16:20:02Z

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

lib/internal/modules/esm/module_job.js

devsnek · 2021-03-21T18:18:32Z

lib/internal/modules/esm/module_job.js

+            '. 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"`.';


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'

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

node/lib/internal/modules/esm/module_job.js

Lines 128 to 134 in 5318e53

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.

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.)

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.

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.

devsnek · 2021-03-21T18:19:14Z

lib/internal/modules/esm/module_job.js

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


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.

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?

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.

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?

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

aduh95 · 2021-03-25T08:48:03Z

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

guybedford

This is a great one, thank you!

guybedford · 2021-03-25T14:16:29Z

lib/internal/modules/esm/module_job.js

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


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.

guybedford · 2021-03-25T14:19:24Z

lib/internal/modules/esm/module_job.js

+        e.message += ' in ES module scope';
+
+        if (StringPrototypeStartsWith(e.message, 'require ')) {
+          e.message += ', you can use import instead';


Perhaps - Use "import" instead or

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

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

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

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

lib/internal/modules/esm/resolve.js

devsnek · 2021-03-25T14:36:39Z

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 · 2021-03-25T14:38:31Z

@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 · 2021-03-25T14:39:45Z

@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 · 2021-03-25T14:42:11Z

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

lib/internal/modules/esm/module_job.js

ljharb · 2021-03-25T14:51:11Z

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

aduh95 · 2021-04-02T09:21:57Z

@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.

Dismissing due to unresponsiveness.

aduh95 · 2021-04-11T17:42:37Z

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

nodejs-github-bot · 2021-04-11T17:43:41Z

CI: https://ci.nodejs.org/job/node-test-pull-request/37387/

nodejs-github-bot · 2021-04-13T10:41:28Z

CI: https://ci.nodejs.org/job/node-test-pull-request/37428/

Fixes: nodejs#33741 PR-URL: nodejs#37852 Reviewed-By: Guy Bedford <guybedford@gmail.com>

aduh95 · 2021-04-13T17:07:48Z

Landed in 7b2bad4

Fixes: #33741 PR-URL: #37852 Reviewed-By: Guy Bedford <guybedford@gmail.com>

nodejs-github-bot added the needs-ci PRs that need a full CI run. label Mar 21, 2021

aduh95 added the esm Issues and PRs related to the ECMAScript Modules implementation. label Mar 21, 2021

ljharb reviewed Mar 21, 2021

View reviewed changes

lib/internal/modules/esm/module_job.js Outdated Show resolved Hide resolved

ljharb reviewed Mar 21, 2021

View reviewed changes

lib/internal/modules/esm/module_job.js Outdated Show resolved Hide resolved

aduh95 force-pushed the require-not-defined-error branch from b732c67 to 5a26936 Compare March 21, 2021 16:18

aduh95 changed the title ~~module: clarify require not defined error message~~ module: clarify CJS global-like variables not defined error message Mar 21, 2021

ljharb reviewed Mar 21, 2021

View reviewed changes

lib/internal/modules/esm/module_job.js Outdated Show resolved Hide resolved

devsnek previously requested changes Mar 21, 2021

View reviewed changes

guybedford approved these changes Mar 25, 2021

View reviewed changes

guybedford reviewed Mar 25, 2021

View reviewed changes

lib/internal/modules/esm/resolve.js Outdated Show resolved Hide resolved

guybedford reviewed Mar 25, 2021

View reviewed changes

lib/internal/modules/esm/module_job.js Outdated Show resolved Hide resolved

guybedford force-pushed the master branch from dc5a5da to 8e46568 Compare March 29, 2021 21:33

aduh95 mentioned this pull request Apr 2, 2021

module: improve error message for invalid data URL #37701

Merged

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

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

7b2bad4

Fixes: nodejs#33741 PR-URL: nodejs#37852 Reviewed-By: Guy Bedford <guybedford@gmail.com>

aduh95 force-pushed the require-not-defined-error branch from 46bdb40 to 7b2bad4 Compare April 13, 2021 17:07

aduh95 merged commit 7b2bad4 into nodejs:master Apr 13, 2021

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

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

d9d995e

Fixes: #33741 PR-URL: #37852 Reviewed-By: Guy Bedford <guybedford@gmail.com>

BethGriggs mentioned this pull request Apr 16, 2021

2021-04-20, Version 16.0.0 (Current) #37678

Merged

targos pushed a commit that referenced this pull request May 30, 2021

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

3bdb24f

Fixes: #33741 PR-URL: #37852 Reviewed-By: Guy Bedford <guybedford@gmail.com>

targos pushed a commit that referenced this pull request Jun 5, 2021

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

41722ee

Fixes: #33741 PR-URL: #37852 Reviewed-By: Guy Bedford <guybedford@gmail.com>

targos mentioned this pull request Jun 6, 2021

v14.17.1 release proposal #38948

Merged

targos pushed a commit that referenced this pull request Jun 11, 2021

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

bd0d964

Fixes: #33741 PR-URL: #37852 Reviewed-By: Guy Bedford <guybedford@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

aduh95 commented Mar 21, 2021 •

edited

Loading

aduh95 commented Mar 21, 2021

devsnek Mar 21, 2021

aduh95 Mar 21, 2021

GeoffreyBooth Mar 22, 2021

ljharb Mar 22, 2021

aduh95 Mar 22, 2021

devsnek Mar 21, 2021

aduh95 Mar 21, 2021

guybedford Mar 25, 2021

aduh95 Mar 25, 2021

guybedford Mar 25, 2021

aduh95 commented Mar 25, 2021

guybedford left a comment

guybedford Mar 25, 2021

guybedford Mar 25, 2021

aduh95 Mar 25, 2021 •

edited

Loading

guybedford Mar 25, 2021

guybedford Mar 25, 2021

aduh95 Mar 25, 2021

devsnek commented Mar 25, 2021

guybedford commented Mar 25, 2021

devsnek commented Mar 25, 2021

guybedford commented Mar 25, 2021

ljharb commented Mar 25, 2021

aduh95 commented Apr 2, 2021

aduh95 commented Apr 11, 2021

nodejs-github-bot commented Apr 11, 2021

nodejs-github-bot commented Apr 13, 2021

aduh95 commented Apr 13, 2021

	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` : ''}`;

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

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

Conversation

aduh95 commented Mar 21, 2021 • edited Loading

aduh95 commented Mar 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduh95 commented Mar 25, 2021

guybedford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduh95 Mar 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devsnek commented Mar 25, 2021

guybedford commented Mar 25, 2021

devsnek commented Mar 25, 2021

guybedford commented Mar 25, 2021

ljharb commented Mar 25, 2021

aduh95 commented Apr 2, 2021

aduh95 commented Apr 11, 2021

nodejs-github-bot commented Apr 11, 2021

nodejs-github-bot commented Apr 13, 2021

aduh95 commented Apr 13, 2021

aduh95 commented Mar 21, 2021 •

edited

Loading

aduh95 Mar 25, 2021 •

edited

Loading