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

Combine ERR_REQUIRE_ESM with warning #30599

Closed
guybedford opened this issue Nov 22, 2019 · 8 comments
Closed

Combine ERR_REQUIRE_ESM with warning #30599

guybedford opened this issue Nov 22, 2019 · 8 comments
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. good first issue Issues that are suitable for first-time contributors. help wanted Issues that need assistance from volunteers or PRs that need help to proceed.

Comments

@guybedford
Copy link
Contributor

Now that modules are unflagged, the warning for ERR_REQUIRE_ESM when loading a .js file that would be interpreted as a module is being thrown along with the warning message in https://github.com/nodejs/node/blob/master/lib/internal/modules/cjs/loader.js#L1193.

This hinders approaches that want to use this error to explicitly check for support.

Ideally the warning can just be combined with the error message.

@guybedford guybedford added the esm Issues and PRs related to the ECMAScript Modules implementation. label Nov 22, 2019
@guybedford guybedford added good first issue Issues that are suitable for first-time contributors. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. labels Nov 25, 2019
@juanarbol
Copy link
Member

I'm gonna take this one.

@guybedford
Copy link
Contributor Author

@juanarbol that would be really great - just let me know if I can help.

@juanarbol
Copy link
Member

@guybedford sure! I'll ask you for some help if needed (I think i'll need it)

@juanarbol
Copy link
Member

So, we still needing to throws the exception, but now, the warning should contain error (ERR_REQUIRE_ESM) message.

@guybedford
Copy link
Contributor Author

@juamedgod yes the error should contain in the message the descriptive contents of what is currently in the warning.

@juanarbol
Copy link
Member

Jm, I'm misunderstanding something.

So, we need to emit the current warning as is, but with the message of ERR_REQUIRE_ESM, and then redefine the message of the error to the same emmited in the warning. so new ERR_REQUIRE_ESM().message = 'same warning message emitted'

In a most short way; the warning message emitted, should be the current message concatenated with ERR_REQUIRE_ESM message, and the message of the trowned ERR_REQUIRE_ESM should the same of the emitted warning.

Am I right?

@guybedford
Copy link
Contributor Author

So at the moment we provide both a warning and an error message.

The error message should continue to be thrown, but the useful information from the warning can be folded into the error message itself.

This way it will still be shown to users, but libraries can wrap the error and swallow it if they need, whereas the warning cannot be silenced without disabling all warnings in Node.js.

@juanarbol
Copy link
Member

juanarbol commented Nov 28, 2019

This is what I've done:

try {
  const a = require('./mod/file2.js')
  console.log(a)
} catch (err) {
  console.log('\nError message: ')
  console.log(err.message)
}
(node:91392) Warning: require() of ES modules is not supported.
require() of /Users/juanjose/Documents/GitHub/Node/node/mod/file2.js from /Users/juanjose/Documents/GitHub/Node/node/file.js is an ES module file as it is a .js file whose nearest parent package.json contains "type": "module" which defines all .js files in that package scope as ES modules.
Instead rename file2.js to end in .cjs, change the requiring code to use import(), or remove "type": "module" from /Users/juanjose/Documents/GitHub/Node/node/mod/package.json.
Must use import to load ES Module: /Users/juanjose/Documents/GitHub/Node/node/mod/file2.js

Error message:
Must use import to load ES Module: /Users/juanjose/Documents/GitHub/Node/node/mod/file2.js
require() of ES modules is not supported.
require() of /Users/juanjose/Documents/GitHub/Node/node/mod/file2.js from /Users/juanjose/Documents/GitHub/Node/node/file.js is an ES module file as it is a .js file whose nearest parent package.json contains "type": "module" which defines all .js files in that package scope as ES modules.
Instead rename file2.js to end in .cjs, change the requiring code to use import(), or remove "type": "module" from /Users/juanjose/Documents/GitHub/Node/node/mod/package.json.

Is this the expected behaviour?

juanarbol added a commit to juanarbol/node that referenced this issue Nov 28, 2019
targos pushed a commit that referenced this issue Dec 9, 2019
PR-URL: #30694
Fixes: #30599
Reviewed-By: Guy Bedford <guybedford@gmail.com>
MylesBorins pushed a commit that referenced this issue Jan 12, 2020
PR-URL: #30694
Fixes: #30599
Reviewed-By: Guy Bedford <guybedford@gmail.com>
BethGriggs pushed a commit that referenced this issue Feb 6, 2020
PR-URL: #30694
Fixes: #30599
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
esm Issues and PRs related to the ECMAScript Modules implementation. good first issue Issues that are suitable for first-time contributors. help wanted Issues that need assistance from volunteers or PRs that need help to proceed.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants