-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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 to package.exports docs #33074
Conversation
If package authors don't explicitly include all previously supported entry points introducing package.exports will be a Semver-Major change. Add a warning about this behavior and offer two potential solutions for module authors. Refs: then/is-promise#20
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.
LGTM!
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.
Spotted some grammatical and syntactical errors, see the suggestions.
Co-Authored-By: Mark Wubben <mark@novemberborn.net>
Co-Authored-By: Mark Wubben <mark@novemberborn.net>
Co-Authored-By: Geoffrey Booth <GeoffreyBooth@users.noreply.github.com>
Co-Authored-By: Geoffrey Booth <GeoffreyBooth@users.noreply.github.com>
Co-Authored-By: Geoffrey Booth <GeoffreyBooth@users.noreply.github.com>
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 again @MylesBorins for this important docs PR... I do feel quite strongly about this one so just making my feedback explicit here:
- If users really want it not to be a break we can encourage carefully listing all subpaths be provided explicitly, including the package.json itself. In theory other private subpaths are private APIs so it is the fault of the importer if things don't work out there.
- Otherwise, we should encourage users to make exports a major change if they are in doubt (instead of just adding "./": "./"). Conditional exports makes that break small in still supporting CJS, so that such a small break can be palatable.
- I would really prefer we don't mention
"./": "./"
at all, but if we really must then it should come after the above, and with its own caveats.
Let me know if I can clarify further / assist with getting this through. If it would help I could commit directly to this PR as well with suggestions for further feedback.
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.
LGTM although I agree with Guy that it may be nice to de-emphasize ./
or at least call out that it may prevent optimization in tools.
Co-Authored-By: Rob Palmer <rob.palmer2@gmail.com>
I've accepted the proposed change from @robpalme. @guybedford are you open to approving this PR based on the current text? |
Thanks @MylesBorins the wording is looking much better, let me know what you think about including an example. |
Some general thoughts on best practices… Only exporting index file entries is not optimal:
It's not maintainable to have a massive list of file exports in the It’s best to cherry-pick directories instead of files for export. Only files intended to be public should be in an exported directory. Private files should be in separate directories or else nested in a directory (e.g. named IMO the most reasonable way to deal with the dual package hazard is to publish for each piece of the API a single CJS / Despite being a seasoned early experimental-modules adopter and closely watching the final ESM implementation develop I've had to learn so many lessons the hard way; there is a tremendous amount to consider. Having a one-size fits all recommendation for |
Co-authored-by: Rob Palmer <rob.palmer2@gmail.com>
Co-authored-by: Rob Palmer <rob.palmer2@gmail.com>
@guybedford I've made some changes and would like another review from you please |
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.
This is really good. Great work!
I'll land this in 24 hours unless anyone objects |
@guybedford If you would like to add it back please include a suggestion
and if no one objects I'll land it. Perhaps linking to documentation about
the tooling might help alleviate concerns?
…On Mon, May 4, 2020 at 3:57 PM Guy Bedford ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In doc/api/esm.md
<#33074 (comment)>:
>
-This is important with regard to `require`, since `require` of ES module files
-throws an error in all versions of Node.js. To create a package that works both
-in modern Node.js via `import` and `require` and also legacy Node.js versions,
-see [the dual CommonJS/ES module packages section][].
+As a last resort, package encapsulation can be disabled entirely by creating an
+export for the root of the package `"./": "./"`. This will expose every file in
+the package at the cost of disabling encapsulation and the tooling and optimization
+benefits that come with it.
Since these tooling and optimization benefits don't yet exist
All my own tooling very much relies on and uses exports for automatic
optimization, including the next version of jspm. And I always opt-out of
all of these optimizations when ./: ./ is set.
I would have preferred we included this line as when giving advice it is
always helpful to explain the reason for the advice.
That said I understand there has been a silly amount of back and fourth on
this PR so I won't press this.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#33074 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADZYV4I7TLTQMT47DMGOS3RP4MZFANCNFSM4MRC4XPA>
.
|
Thanks @MylesBorins, I've added the suggestion with some slight rewording. @GeoffreyBooth let me know if that works for you? |
Co-authored-by: Guy Bedford <guybedford@gmail.com>
made some more changes PTAL |
If package authors don't explicitly include all previously supported entry points introducing package.exports will be a Semver-Major change. Add a warning about this behavior and offer two potential solutions for module authors. Refs: then/is-promise#20 PR-URL: #33074 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
landed in 1ffd182 |
If package authors don't explicitly include all previously supported entry points introducing package.exports will be a Semver-Major change. Add a warning about this behavior and offer two potential solutions for module authors. Refs: then/is-promise#20 PR-URL: #33074 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
If package authors don't explicitly include all previously supported entry points introducing package.exports will be a Semver-Major change. Add a warning about this behavior and offer two potential solutions for module authors. Refs: then/is-promise#20 PR-URL: #33074 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
If package authors don't explicitly include all previously supported
entry points introducing package.exports will be a Semver-Major change.
Add a warning about this behavior and offer two potential solutions
for module authors.
Refs: then/is-promise#20