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

Jump to bottom

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

Closed
wants to merge 3 commits into from
Closed

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

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.

bmeck
bmeck suggested changes Dec 18, 2019
View reviewed changes
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

@tamias
doc: fix explanation of package.json "type" field
fccd01b 
Remove erroneous reference to files with `.mjs` extension, which are not
affected by the package.json "type" field.
@tamias
doc: further clarify package.json "type" field
1d63497 
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?

GeoffreyBooth
GeoffreyBooth reviewed Dec 31, 2019
View reviewed changes
doc/api/esm.md Outdated Show resolved Hide resolved
@tamias @GeoffreyBooth
doc: change "CommonJS modules" to "CommonJS"
011fe06 
Co-Authored-By: Geoffrey Booth <GeoffreyBooth@users.noreply.github.com>
@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
@tamias @BridgeAR
doc: improve explanation of package.json "type" field
76d4a23 
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 <bradley.meck@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@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
@tamias @BridgeAR
doc: improve explanation of package.json "type" field
d3a8088 
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 <bradley.meck@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@BridgeAR BridgeAR mentioned this pull request Jan 7, 2020
Merged
targos pushed a commit that referenced this pull request Jan 14, 2020
@tamias @targos
doc: improve explanation of package.json "type" field
39dbc10 
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 <bradley.meck@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@targos targos mentioned this pull request Jan 15, 2020
Closed
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
@tamias @BethGriggs
doc: improve explanation of package.json "type" field
deb1a59 
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 <bradley.meck@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb approved these changes

@bmeck bmeck bmeck approved these changes

@GeoffreyBooth GeoffreyBooth GeoffreyBooth approved these changes

@Trott Trott Trott approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@guybedford guybedford Awaiting requested review from guybedford

@devsnek devsnek Awaiting requested review from devsnek

@MylesBorins MylesBorins Awaiting requested review from MylesBorins

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@tamias @BridgeAR @GeoffreyBooth @gireeshpunathil @bmeck @Trott @ljharb @nodejs-github-bot