doc: packages docs feedback #35370

guybedford · 2020-09-27T05:16:05Z

This PR brings through the deferred feedback from #34748 from myself and @isaacs.

I carefully ensured to mention all items brought up there in this PR including renaming "package scope" to "package boundary" and adding a summary section to the list of fields.

Further review suggestions very welcome. This should then unblock #34970 and subsequent PRs from release.

@nodejs/modules-active-members @nodejs/documentation @aduh95

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

nodejs-github-bot · 2020-09-27T05:16:10Z

Review requested:

@nodejs/modules

guybedford · 2020-09-27T05:17:19Z

doc/api/packages.md

-
-Since `"not:valid"` is not a valid specifier, `"./submodule.js"` is used
-instead as the fallback, as if it were the only target.
-


As per @isaacs's comment this also removes documentation on package fallbacks. This may or may not be controversial to the modules group though but I tend to agree with the sentiment of simplifying the outward documentation for this already-complex field.

DerekNonGeneric

Thanks for following through.

aduh95

Thanks a lot for doing this!

doc/api/packages.md

aduh95 · 2020-09-27T07:48:55Z

doc/api/packages.md

-A package could also switch from CommonJS to ES module syntax in a breaking
-change version bump. This has the disadvantage that the newest version
-of the package would only be usable in ES module-supporting versions of Node.js.
+A package could also switch from CommonJS to ES module syntax in a [breaking
+change](https://semver.org/) version bump. This has the disadvantage that the
+newest version of the package would only be usable in ES module-supporting
+versions of Node.js.


Should we add a note to help users find the relevant info? Somebody who's new to Node.js may ask themself what are the ESM-supporting versions.
That could go in a separate PR as I see there is no History section at the top of esm.md.

It sounds like a section on compatibility via a new PR would be useful to users actually. Perhaps esm.md could get a history or we could add a module compatibility section that mentions what levels of ESM support can be expected where.

TODO for myself: add a reference to the support table on #35395 if this PR lands first.

doc/api/packages.md

guybedford · 2020-09-27T11:51:31Z

@aduh95 thanks for the review, feedback added.

doc/api/packages.md

GeoffreyBooth

@guybedford please take a look. After this I don’t think there should be any remaining references to “scope” or “boundary”, please let me know if I missed any.

doc/api/packages.md

Co-authored-by: Geoffrey Booth <[email protected]>

guybedford · 2020-09-28T06:12:25Z

@GeoffreyBooth yes that's all cases of boundary now removed.

GeoffreyBooth

@guybedford Thanks for working with me on this.

Do we want to link the various “nearest parent package.json“ references to the new “package.json and file extensions” section?

guybedford · 2020-09-28T06:39:28Z

Possibly that can be a subsequent PR? Also note that that section no longer covers the lookup algorithm though.

…

On Sun, Sep 27, 2020 at 23:36 Geoffrey Booth ***@***.***> wrote: ***@***.**** approved this pull request. @guybedford <https://github.com/guybedford> Thanks for working with me on this. Do we want to link the various “nearest parent package.json“ references to the new “package.json and file extensions” section? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#35370 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAESFSQEPLEHWVUUD7AINULSIAVE7ANCNFSM4R3KYK3A> .

doc/api/packages.md

GeoffreyBooth · 2020-09-28T06:44:14Z

Possibly that can be a subsequent PR? Also note that that section no longer covers the lookup algorithm though.

Can certainly come later, if we think it’s a good idea at all. Not everything needs to be a link.

And you’re right, it should link to the “"type"“ section. That’s where the algorithm is defined.

Trott · 2020-09-29T12:19:36Z

doc/api/packages.md

+* [`"type"`][] - The package type determining whether to load `.js` files as
+  CommonJS or ES modules.
+* [`"exports"`][] - Package exports and conditional exports. When present,
+  limits which submodules may be loaded from within the package.


Suggested change

limits which submodules may be loaded from within the package.

limits which submodules can be loaded from within the package.

Trott · 2020-09-29T12:23:56Z

doc/api/packages.md

-subpaths of the package will no longer be available to importers under
-`require('pkg/subpath.js')`, and instead they will get a new error,
-`ERR_PACKAGE_PATH_NOT_EXPORTED`.
+When defining the [`"exports"`][] field, all subpaths of the package will be


Suggested change

When defining the [`"exports"`][] field, all subpaths of the package will be

When defining the [`"exports"`][] field, all subpaths of the package are

...or perhaps:

Suggested change

When defining the [`"exports"`][] field, all subpaths of the package will be

When the [`"exports"`][] field is defined, all subpaths of the package are

Trott · 2020-09-29T12:24:22Z

doc/api/packages.md

-`ERR_PACKAGE_PATH_NOT_EXPORTED`.
+When defining the [`"exports"`][] field, all subpaths of the package will be
+encapsulated and no longer available to importers. For example,
+`require('pkg/subpath.js')` would throw an [`ERR_PACKAGE_PATH_NOT_EXPORTED`][]


Suggested change

`require('pkg/subpath.js')` would throw an [`ERR_PACKAGE_PATH_NOT_EXPORTED`][]

`require('pkg/subpath.js')` throws an [`ERR_PACKAGE_PATH_NOT_EXPORTED`][]

Trott

Left some micro-nit suggestions, but this looks good to me with or without them.

PR-URL: #35370 Reviewed-By: Derek Lewis <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Rich Trott <[email protected]>

MylesBorins · 2020-09-29T15:50:38Z

landed in 4d7015f

@Trott I totally missed your suggestions, apologies. Can you include in a follow up? (I can do this too)

PR-URL: #35370 Reviewed-By: Derek Lewis <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Rich Trott <[email protected]>

Trott · 2020-09-30T12:36:03Z

@Trott I totally missed your suggestions, apologies. Can you include in a follow up?

Sure. #35427

None of them were blocking anyway, so no big deal.

Some minor adjustments, mostly moving things to present tense and avoiding the ambiguity of _may_. Refs: nodejs#35370 (comment) PR-URL: nodejs#35427 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]>

Some minor adjustments, mostly moving things to present tense and avoiding the ambiguity of _may_. Refs: #35370 (comment) PR-URL: #35427 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]>

PR-URL: nodejs#35370 Reviewed-By: Derek Lewis <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Rich Trott <[email protected]>

Some minor adjustments, mostly moving things to present tense and avoiding the ambiguity of _may_. Refs: nodejs#35370 (comment) PR-URL: nodejs#35427 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]>

Backport-PR-URL: #35757 PR-URL: #35370 Reviewed-By: Derek Lewis <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Rich Trott <[email protected]>

Some minor adjustments, mostly moving things to present tense and avoiding the ambiguity of _may_. Refs: #35370 (comment) Backport-PR-URL: #35757 PR-URL: #35427 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]>

Backport-PR-URL: #35757 PR-URL: #35370 Reviewed-By: Derek Lewis <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Rich Trott <[email protected]>

Some minor adjustments, mostly moving things to present tense and avoiding the ambiguity of _may_. Refs: #35370 (comment) Backport-PR-URL: #35757 PR-URL: #35427 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]>

PR-URL: nodejs#35370 Reviewed-By: Derek Lewis <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Rich Trott <[email protected]>

Some minor adjustments, mostly moving things to present tense and avoiding the ambiguity of _may_. Refs: nodejs#35370 (comment) PR-URL: nodejs#35427 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]>

doc: packages docs feedback

d054ee0

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Sep 27, 2020

guybedford commented Sep 27, 2020

View reviewed changes

fixups

4f9495c

guybedford mentioned this pull request Sep 27, 2020

doc: move package config docs to separate page #34748

Closed

3 tasks

DerekNonGeneric approved these changes Sep 27, 2020

View reviewed changes

aduh95 approved these changes Sep 27, 2020

View reviewed changes

pr feedbackg

0598dc9

GeoffreyBooth requested changes Sep 27, 2020

View reviewed changes

doc/api/packages.md Outdated Show resolved Hide resolved

keep nearest parent phrases

7df9cd7

GeoffreyBooth reviewed Sep 28, 2020

View reviewed changes

guybedford and others added 3 commits September 27, 2020 23:05

package boundary suggestions

896b1ff

Co-authored-by: Geoffrey Booth <[email protected]>

Apply suggestions from code review

2a19db1

Co-authored-by: Geoffrey Booth <[email protected]>

further feedback

3a35054

remove unused definition

272a18c

guybedford requested a review from GeoffreyBooth September 28, 2020 06:31

GeoffreyBooth approved these changes Sep 28, 2020

View reviewed changes

GeoffreyBooth reviewed Sep 28, 2020

View reviewed changes

doc/api/packages.md Outdated Show resolved Hide resolved

Update doc/api/packages.md

1ebef9a

aymen94 approved these changes Sep 28, 2020

View reviewed changes

aduh95 mentioned this pull request Sep 28, 2020

doc,esm: add history support info #35395

Merged

3 tasks

guybedford added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 29, 2020

Trott reviewed Sep 29, 2020

View reviewed changes

Trott approved these changes Sep 29, 2020

View reviewed changes

MylesBorins pushed a commit that referenced this pull request Sep 29, 2020

doc: packages docs feedback

4d7015f

PR-URL: #35370 Reviewed-By: Derek Lewis <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Rich Trott <[email protected]>

MylesBorins closed this Sep 29, 2020

MylesBorins pushed a commit that referenced this pull request Sep 29, 2020

doc: packages docs feedback

46a4154

PR-URL: #35370 Reviewed-By: Derek Lewis <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Rich Trott <[email protected]>

MylesBorins mentioned this pull request Sep 29, 2020

v14.13.0 proposal #35419

Merged

Trott mentioned this pull request Sep 30, 2020

doc: copyedit packages.md #35427

Merged

2 tasks

aduh95 mentioned this pull request Oct 23, 2020

[v12 backport] ESM (named exports for CJS, package.json docs, subpath export patterns, warning removal) #35757

Closed

3 tasks

aduh95 pushed a commit to aduh95/node that referenced this pull request Oct 23, 2020

doc: packages docs feedback

ee644b4

PR-URL: nodejs#35370 Reviewed-By: Derek Lewis <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Rich Trott <[email protected]>

MylesBorins pushed a commit that referenced this pull request Nov 3, 2020

doc: packages docs feedback

d100516

Backport-PR-URL: #35757 PR-URL: #35370 Reviewed-By: Derek Lewis <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Rich Trott <[email protected]>

MylesBorins mentioned this pull request Nov 3, 2020

v12.20.0 proposal #35950

Merged

MylesBorins pushed a commit that referenced this pull request Nov 16, 2020

doc: packages docs feedback

f507ca9

Backport-PR-URL: #35757 PR-URL: #35370 Reviewed-By: Derek Lewis <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Rich Trott <[email protected]>

joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021

doc: packages docs feedback

bd2ce75

PR-URL: nodejs#35370 Reviewed-By: Derek Lewis <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Rich Trott <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: packages docs feedback #35370

doc: packages docs feedback #35370

guybedford commented Sep 27, 2020

nodejs-github-bot commented Sep 27, 2020

guybedford Sep 27, 2020

DerekNonGeneric left a comment

aduh95 left a comment

aduh95 Sep 27, 2020

guybedford Sep 27, 2020

aduh95 Sep 29, 2020 •

edited

Loading

guybedford commented Sep 27, 2020

GeoffreyBooth left a comment

guybedford commented Sep 28, 2020

GeoffreyBooth left a comment

guybedford commented Sep 28, 2020 via email

GeoffreyBooth commented Sep 28, 2020

Trott Sep 29, 2020

Trott Sep 29, 2020

Trott Sep 29, 2020

Trott left a comment

MylesBorins commented Sep 29, 2020

Trott commented Sep 30, 2020


		Since `"not:valid"` is not a valid specifier, `"./submodule.js"` is used
		instead as the fallback, as if it were the only target.

	limits which submodules may be loaded from within the package.
	limits which submodules can be loaded from within the package.

	When defining the [`"exports"`][] field, all subpaths of the package will be
	When defining the [`"exports"`][] field, all subpaths of the package are

	When defining the [`"exports"`][] field, all subpaths of the package will be
	When the [`"exports"`][] field is defined, all subpaths of the package are

	`require('pkg/subpath.js')` would throw an [`ERR_PACKAGE_PATH_NOT_EXPORTED`][]
	`require('pkg/subpath.js')` throws an [`ERR_PACKAGE_PATH_NOT_EXPORTED`][]

doc: packages docs feedback #35370

doc: packages docs feedback #35370

Conversation

guybedford commented Sep 27, 2020

Checklist

nodejs-github-bot commented Sep 27, 2020

guybedford Sep 27, 2020

Choose a reason for hiding this comment

DerekNonGeneric left a comment

Choose a reason for hiding this comment

aduh95 left a comment

Choose a reason for hiding this comment

aduh95 Sep 27, 2020

Choose a reason for hiding this comment

guybedford Sep 27, 2020

Choose a reason for hiding this comment

aduh95 Sep 29, 2020 • edited Loading

Choose a reason for hiding this comment

guybedford commented Sep 27, 2020

GeoffreyBooth left a comment

Choose a reason for hiding this comment

guybedford commented Sep 28, 2020

GeoffreyBooth left a comment

Choose a reason for hiding this comment

guybedford commented Sep 28, 2020 via email

GeoffreyBooth commented Sep 28, 2020

Trott Sep 29, 2020

Choose a reason for hiding this comment

Trott Sep 29, 2020

Choose a reason for hiding this comment

Trott Sep 29, 2020

Choose a reason for hiding this comment

Trott left a comment

Choose a reason for hiding this comment

MylesBorins commented Sep 29, 2020

Trott commented Sep 30, 2020

aduh95 Sep 29, 2020 •

edited

Loading