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

doc: fix explanation of package.json "type" field #27516

Closed
wants to merge 3 commits into from

Conversation

tamias
Copy link
Contributor

@tamias tamias commented May 1, 2019

Remove erroneous reference to files with .mjs extension, which are not affected by the "type" field.

Checklist

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label May 1, 2019
@BridgeAR
Copy link
Member

@nodejs/modules-active-members PTAL

@GeoffreyBooth
Copy link
Member

I'm not sure that this is an improvement. I think it's important for users to know that both .js and .mjs files are treated as ES modules within the module package scope. That's not as obvious if .mjs isn't mentioned, and potentially confusing if .mjs is mentioned only as being unaffected by the field.

@tamias
Copy link
Contributor Author

tamias commented May 13, 2019

@GeoffreyBooth .mjs files are already mentioned earlier in the file:

Once enabled, Node.js will treat the following as ES modules when passed to
node as the initial input, or when referenced by import statements within
ES module code:

  • Files ending in .mjs.
  • Files ending in .js, or extensionless files, when the nearest parent
    package.json file contains a top-level field "type" with a value of
    "module".
  • Strings passed in as an argument to --eval or --print, or piped to
    node via STDIN, with the flag --input-type=module.

The edited paragraph documents the "type" field specifically. The original text implies that the "type" field affects how .mjs files are loaded, which contradicts the quoted text.

@gireeshpunathil
Copy link
Member

ping @GeoffreyBooth

@GeoffreyBooth
Copy link
Member

I disagree with this change. While the field may not affect .mjs files, I think it's important to say that they still behave as ES modules in a type: module scope.

Copy link
Member

@bmeck bmeck left a comment

Choose a reason for hiding this comment

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

I do not find the removal to improve clarity or ease of searching for information.

doc/api/esm.md Show resolved Hide resolved
@tamias
Copy link
Contributor Author

tamias commented Dec 19, 2019

How about, in addition to this change, adding a sentence in the type section along the lines of:

Regardless of the value of "type", .mjs files are always treated as ES modules and .cjs files are always treated as CommonJS modules.

@bmeck
Copy link
Member

bmeck commented Dec 19, 2019

@tamias that seems fine

@Trott
Copy link
Member

Trott commented Dec 31, 2019

@bmeck Does this change look OK to you now?

@tamias Can you give this a rebase to eliminate the conflict?

@bmeck
Copy link
Member

bmeck commented Dec 31, 2019

@Trott lgtm

Remove erroneous reference to files with `.mjs` extension, which are not
affected by the package.json "type" field.
Added sentence about the type field not affecting .mjs and .cjs files.
@tamias
Copy link
Contributor Author

tamias commented Dec 31, 2019

I was already working on the rebase. 😛

A separate paragraph had already been added at the end of that doc section; in the rebase I put my sentence after that paragraph. Still look okay that way?

@bmeck
Copy link
Member

bmeck commented Dec 31, 2019

seems fine

@Trott Trott added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Dec 31, 2019
@Trott
Copy link
Member

Trott commented Dec 31, 2019

@GeoffreyBooth Does this change look OK to you now? Or is it still problematic from your point of view?

doc/api/esm.md Outdated Show resolved Hide resolved
@GeoffreyBooth
Copy link
Member

Looks good now, thanks!

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 2, 2020
BridgeAR pushed a commit that referenced this pull request Jan 3, 2020
Regardless of the value of the `"type"` field, `.mjs` files are
always treated as ES modules and `.cjs` files are always treated
as CommonJS.

PR-URL: #27516
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@BridgeAR
Copy link
Member

BridgeAR commented Jan 3, 2020

Landed in 76d4a23

@tamias congratulations on your first commit to Node.js! 🎉

@BridgeAR BridgeAR closed this Jan 3, 2020
BridgeAR pushed a commit that referenced this pull request Jan 3, 2020
Regardless of the value of the `"type"` field, `.mjs` files are
always treated as ES modules and `.cjs` files are always treated
as CommonJS.

PR-URL: #27516
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Jan 7, 2020
targos pushed a commit that referenced this pull request Jan 14, 2020
Regardless of the value of the `"type"` field, `.mjs` files are
always treated as ES modules and `.cjs` files are always treated
as CommonJS.

PR-URL: #27516
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
Regardless of the value of the `"type"` field, `.mjs` files are
always treated as ES modules and `.cjs` files are always treated
as CommonJS.

PR-URL: #27516
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
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. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants