★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/27516Nouvelle recherche | Portail wanayoo
Skip to content

/ node

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
Closed

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

wants to merge 3 commits into from

Conversation

@tamias
Copy link
Contributor

tamias commented May 1, 2019

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

Checklist
@BridgeAR BridgeAR requested review from guybedford, devsnek and MylesBorins May 3, 2019
@BridgeAR

This comment has been minimized.

Sign in to view
Copy link
Member

BridgeAR commented May 13, 2019

@nodejs/modules-active-members PTAL
@ljharb
ljharb approved these changes May 13, 2019
View changes
@GeoffreyBooth

This comment has been minimized.

Sign in to view
Copy link
Member

GeoffreyBooth commented May 13, 2019

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

This comment has been minimized.

Sign in to view
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.
@Trott Trott force-pushed the nodejs:master branch from 1ecc406 to 49cf67e Sep 17, 2019
@gireeshpunathil

This comment has been minimized.

Sign in to view
Copy link
Member

gireeshpunathil commented Dec 18, 2019

ping @GeoffreyBooth
@GeoffreyBooth

This comment has been minimized.

Sign in to view
Copy link
Member

GeoffreyBooth commented Dec 18, 2019

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 requested changes Dec 18, 2019
View changes
Copy link
Member

bmeck left a comment

I do not find the removal to improve clarity or ease of searching for information.
doc/api/esm.md Show resolved Hide resolved
@tamias

This comment has been minimized.

Sign in to view
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

This comment has been minimized.

Sign in to view
Copy link
Member

bmeck commented Dec 19, 2019

@tamias that seems fine
@devnexen devnexen force-pushed the nodejs:master branch from e8a4568 to 5289f80 Dec 26, 2019
@Trott

This comment has been minimized.

Sign in to view
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
bmeck approved these changes Dec 31, 2019
View changes
@bmeck

This comment has been minimized.

Sign in to view
Copy link
Member

bmeck commented Dec 31, 2019

@Trott lgtm
tamias added 2 commits May 1, 2019
@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
Loading status checks…
1d63497 
Added sentence about the type field not affecting .mjs and .cjs files.
@tamias tamias force-pushed the tamias:patch-1 branch to 1d63497 Dec 31, 2019
@tamias

This comment has been minimized.

Sign in to view
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

This comment has been minimized.

Sign in to view
Copy link
Member

bmeck commented Dec 31, 2019

seems fine
@Trott
Trott approved these changes Dec 31, 2019
View changes
@Trott Trott added author ready and removed author ready labels Dec 31, 2019
@Trott

This comment has been minimized.

Sign in to view
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 changes
doc/api/esm.md Outdated Show resolved Hide resolved
@tamias @GeoffreyBooth
doc: change "CommonJS modules" to "CommonJS"
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
011fe06 
Co-Authored-By: Geoffrey Booth <GeoffreyBooth@users.noreply.github.com>
@GeoffreyBooth

This comment has been minimized.

Sign in to view
Copy link
Member

GeoffreyBooth commented Jan 2, 2020

Looks good now, thanks!
@GeoffreyBooth
GeoffreyBooth approved these changes Jan 2, 2020
View changes
@BridgeAR
BridgeAR approved these changes Jan 2, 2020
View changes
BridgeAR added a commit that referenced this pull request Jan 3, 2020
@tamias @BridgeAR
doc: improve explanation of package.json "type" field
Unverified
The committer email address is not verified.
Learn about signing commits
Loading status checks…
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

This comment has been minimized.

Sign in to view
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 added a commit that referenced this pull request Jan 3, 2020
@tamias @BridgeAR
doc: improve explanation of package.json "type" field
Unverified
The committer email address is not verified.
Learn about signing commits
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 added a commit that referenced this pull request Jan 14, 2020
@tamias @targos
doc: improve explanation of package.json "type" field
Verified
This commit was signed with a verified signature.
targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
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 added a commit that referenced this pull request Feb 6, 2020
@tamias @BethGriggs
doc: improve explanation of package.json "type" field
Verified
This commit was signed with a verified signature.
BethGriggs Bethany Nicolle Griggs
GPG key ID: D7062848A1AB005C Learn about signing commits
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
saitonakamura pushed a commit to saitonakamura/node that referenced this pull request Apr 7, 2020
@tamias Michael Bashurov
doc: improve explanation of package.json "type" field
19054f5 
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: nodejs#27516
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb

@bmeck bmeck

@GeoffreyBooth GeoffreyBooth

@Trott Trott

@BridgeAR BridgeAR

@MylesBorins MylesBorins

@guybedford guybedford

@devsnek devsnek

Assignees
No one assigned
Labels
author ready doc
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
@tamias @BridgeAR @GeoffreyBooth @gireeshpunathil @bmeck @Trott @ljharb @nodejs-github-bot
You can’t perform that action at this time.