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: warn on require of .js inside type: module #29909

Closed
wants to merge 3 commits into from

Conversation

guybedford
Copy link
Contributor

@guybedford guybedford commented Oct 9, 2019

This is a follow-up to the PR made in #29732 ensuring a warning is emitted when violating the "type": "module" expected module format.

This was discussed at today's modules group meeting as a starting point for handling this case, and then whether or not an error will be thrown when modules are unflagged is still to be discussed further.

Example output:

(node:28347) Warning: require() of ES modules is not supported.
require() of /home/guybedford/Projects/node/test/fixtures/es-modules/package-type-module/cjs.js from /home/guybedford/Projects/node/test/fixtures/es-modules/cjs-esm.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 cjs.js to end in .cjs, change the requiring code to use import(), or remove "type": "module" from /home/guybedford/Projects/node/test/fixtures/es-modules/package-type-module/package.json.

@nodejs/modules-active-members

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

Copy link
Contributor

@jkrems jkrems left a comment

Choose a reason for hiding this comment

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

LGTM

@guybedford guybedford added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 10, 2019
@guybedford guybedford added esm Issues and PRs related to the ECMAScript Modules implementation. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Oct 10, 2019
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@guybedford guybedford added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 11, 2019
@guybedford guybedford mentioned this pull request Oct 11, 2019
2 tasks
@jkrems
Copy link
Contributor

jkrems commented Oct 11, 2019

Revised message SGTM!

guybedford added a commit that referenced this pull request Oct 11, 2019
PR-URL: #29909
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@guybedford
Copy link
Contributor Author

Landed in aca1c28.

@guybedford guybedford closed this Oct 11, 2019
targos pushed a commit that referenced this pull request Nov 8, 2019
PR-URL: #29909
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos pushed a commit that referenced this pull request Nov 10, 2019
PR-URL: #29909
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants