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: update packages documentation for Node.js 12 EOL #43375

Closed
wants to merge 31 commits into from

Conversation

guybedford
Copy link
Contributor

@guybedford guybedford commented Jun 11, 2022

This updates the packages documentation to update some of the modules "exports" guidance with the Node.js 12 EOL, specifically:

  • "main" is no longer required as a fallback when using "exports", instead we explain how to decide which to use based on version support. I have gone quite strongly here in recommending the "exports" pattern - this guidance could certainly be relaxed to be more generous to keeping "main" around as well.
  • In all "exports" examples, I've shifted to the pattern of including the file extension in the subpath, as has kind of come to be seen as the better approach to ensure interop with import maps. I've mentioned some brief notes around this and updated all the examples, including the patterns examples to use pattern trailers.
  • I've updated the version history on subpath patterns to include the history for imports pattern support and pattern trailers support.
  • I've moved the exports sugar section higher up, this should help make the linear flow a little easier to understand since we jump between forms.
  • Added a new section on extensionless versus extensioned subpaths

/cc @nodejs/modules

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Jun 11, 2022
doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
Co-authored-by: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
@aduh95
Copy link
Contributor

aduh95 commented Jun 14, 2022

It looks like make test-doc is failing.

doc/api/packages.md Outdated Show resolved Hide resolved
".": "./main.js",
"./submodule": "./src/submodule.js"
".": "./index.js",
"./submodule.js": "./src/submodule.js"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"./submodule.js": "./src/submodule.js"
"./submodule": "./src/submodule.js"

Copy link
Member

Choose a reason for hiding this comment

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

i still think these examples shouldn't be changed to add the extension.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don’t really understand what’s motivating this change either, if anything that adds more bloat to the import maps – I mean it adds 3 bytes; jokes aside, it could give the reader the impression that using an extension is required when defining an export path, and/or confuse needlessly the package consumers on why the is no submodule.js file at the trot of the package – whereas extensionless makes a good job signaling we’re not dealing with actual paths imo.
(Same thing for the change main.js -> index.js, I don’t think it’s necessary)

That being said, I don’t feel strongly about this, so feel free to disregard.

}
}
```

Now only the defined subpath in [`"exports"`][] can be imported by a consumer:

```js
import submodule from 'es-module-package/submodule';
import submodule from 'es-module-package/submodule.js';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import submodule from 'es-module-package/submodule.js';
import submodule from 'es-module-package/submodule';

doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
@ljharb
Copy link
Member

ljharb commented Jun 15, 2022

The docs should only reflect node's present, not node's (quite uncertain) future wrt import maps.

When node supports import maps, and the feature is sadly unable to support the most common node ecosystem pattern, then that would be the appropriate time to have opinionated docs - not now, when it's still just wishful thinking from a few collaborators.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Thanks for the iteration here; I think this does a great job minimizing editorializing in the node docs. LGTM.

".": "./main.js",
"./submodule": "./src/submodule.js"
".": "./index.js",
"./submodule.js": "./src/submodule.js"
Copy link
Member

Choose a reason for hiding this comment

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

i still think these examples shouldn't be changed to add the extension.

doc/api/packages.md Outdated Show resolved Hide resolved
doc/api/packages.md Outdated Show resolved Hide resolved
@guybedford
Copy link
Contributor Author

Added some final clarifications to the extensions guidance and hoping to land this soon.

doc/api/packages.md Outdated Show resolved Hide resolved
".": "./main.js",
"./submodule": "./src/submodule.js"
".": "./index.js",
"./submodule.js": "./src/submodule.js"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don’t really understand what’s motivating this change either, if anything that adds more bloat to the import maps – I mean it adds 3 bytes; jokes aside, it could give the reader the impression that using an extension is required when defining an export path, and/or confuse needlessly the package consumers on why the is no submodule.js file at the trot of the package – whereas extensionless makes a good job signaling we’re not dealing with actual paths imo.
(Same thing for the change main.js -> index.js, I don’t think it’s necessary)

That being said, I don’t feel strongly about this, so feel free to disregard.

doc/api/packages.md Outdated Show resolved Hide resolved
guybedford added a commit that referenced this pull request Jun 15, 2022
PR-URL: #43375
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@guybedford
Copy link
Contributor Author

Landed in 74716ad.

@guybedford guybedford closed this Jun 15, 2022
@guybedford guybedford deleted the esm-docs-12-deprecate branch June 15, 2022 20:44
danielleadams pushed a commit that referenced this pull request Jun 16, 2022
PR-URL: #43375
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@danielleadams danielleadams mentioned this pull request Jun 16, 2022
@targos
Copy link
Member

targos commented Jul 12, 2022

This needs a backport PR to land on v16.x-staging. There is a conflict because of the "Subpath folder mappings" section and I'm not sure where to put it.

@guybedford
Copy link
Contributor Author

Thanks, I've posted a backport in #43809.

targos pushed a commit that referenced this pull request Jul 18, 2022
PR-URL: #43375
Backport-PR-URL: #43809
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Jul 31, 2022
PR-URL: #43375
Backport-PR-URL: #43809
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
PR-URL: nodejs/node#43375
Backport-PR-URL: nodejs/node#43809
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants