-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
Conversation
Review requested:
|
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com> Co-authored-by: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
It looks like |
".": "./main.js", | ||
"./submodule": "./src/submodule.js" | ||
".": "./index.js", | ||
"./submodule.js": "./src/submodule.js" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"./submodule.js": "./src/submodule.js" | |
"./submodule": "./src/submodule.js" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import submodule from 'es-module-package/submodule.js'; | |
import submodule from 'es-module-package/submodule'; |
Co-authored-by: Jordan Harband <ljharb@gmail.com>
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. |
There was a problem hiding this 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" |
There was a problem hiding this comment.
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.
Added some final clarifications to the extensions guidance and hoping to land this soon. |
".": "./main.js", | ||
"./submodule": "./src/submodule.js" | ||
".": "./index.js", | ||
"./submodule.js": "./src/submodule.js" |
There was a problem hiding this comment.
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.
PR-URL: #43375 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Landed in 74716ad. |
PR-URL: #43375 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
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. |
Thanks, I've posted a backport in #43809. |
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>
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."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./cc @nodejs/modules