GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — doc: update to package.exports docs by MylesBorins · Pull Request #33074 · nodejs/node

Conversation

@MylesBorins

@MylesBorins MylesBorins commented Apr 26, 2020

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji brainkim reacted with eyes emoji
All reactions
  • 👍 1 reaction
  • 👀 1 reaction
@MylesBorins
doc: explicitly doc package.exports is breaking
4e4f086 
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
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to Node.js documentation. esm Issues and PRs related to the ECMAScript Modules implementation. labels Apr 26, 2020
@MylesBorins
MylesBorins requested review from guybedford and hybrist and removed request for guybedford April 26, 2020 06:49
@MylesBorins MylesBorins mentioned this pull request Apr 26, 2020
"exports" config then/is-promise#20
Closed
ljharb
ljharb reviewed Apr 26, 2020
View reviewed changes

@ljharb ljharb left a comment

Copy link
Copy Markdown
Member

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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
targos
targos reviewed Apr 26, 2020
View reviewed changes
Comment thread doc/api/esm.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
novemberborn
novemberborn reviewed Apr 26, 2020
View reviewed changes

@novemberborn novemberborn left a comment

Copy link
Copy Markdown

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread doc/api/esm.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread doc/api/esm.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
@richardlau richardlau mentioned this pull request Apr 26, 2020
[v12.x] Backport unflag --experimental-modules #33055
Closed
GeoffreyBooth
GeoffreyBooth reviewed Apr 26, 2020
View reviewed changes
Comment thread doc/api/esm.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread doc/api/esm.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread doc/api/esm.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
MylesBorins and others added 5 commits April 26, 2020 22:14
@MylesBorins @novemberborn
Update doc/api/esm.md
1068cfb 
Co-Authored-By: Mark Wubben <[email protected]>
@MylesBorins @novemberborn
Update doc/api/esm.md
c1f5310 
Co-Authored-By: Mark Wubben <[email protected]>
@MylesBorins @GeoffreyBooth
Update doc/api/esm.md
dd2ba5b 
Co-Authored-By: Geoffrey Booth <[email protected]>
@MylesBorins @GeoffreyBooth
Update doc/api/esm.md
0fc7b73 
Co-Authored-By: Geoffrey Booth <[email protected]>
@MylesBorins @GeoffreyBooth
Update doc/api/esm.md
5508ad8 
Co-Authored-By: Geoffrey Booth <[email protected]>
GeoffreyBooth
GeoffreyBooth previously approved these changes Apr 27, 2020
View reviewed changes
guybedford
guybedford reviewed Apr 27, 2020
View reviewed changes
Comment thread doc/api/esm.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
robpalme
robpalme reviewed Apr 27, 2020
View reviewed changes
Comment thread doc/api/esm.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread doc/api/esm.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
guybedford
guybedford requested changes Apr 27, 2020
View reviewed changes

@guybedford guybedford left a comment

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
hybrist
hybrist previously approved these changes Apr 27, 2020
View reviewed changes

@hybrist hybrist left a comment

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@MylesBorins @robpalme
Update doc/api/esm.md
2acd426 
Co-Authored-By: Rob Palmer <[email protected]>
@MylesBorins

MylesBorins commented Apr 28, 2020

Copy link
Copy Markdown
Contributor Author

I've accepted the proposed change from @robpalme.

@guybedford are you open to approving this PR based on the current text?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
guybedford
guybedford reviewed Apr 28, 2020
View reviewed changes
Comment thread doc/api/esm.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
@guybedford

guybedford commented Apr 28, 2020

Copy link
Copy Markdown
Contributor

Thanks @MylesBorins the wording is looking much better, let me know what you think about including an example.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jaydenseric

jaydenseric commented Apr 28, 2020

Copy link
Copy Markdown
Contributor

Some general thoughts on best practices…

Only exporting index file entries is not optimal:

  • This is a breaking change for projects already using deep imports, whether the paths were documented as part of the "public" API or not.
  • Deep imports to specific files can be important for people who care about efficient bundling, or even efficiency without bundling.

It's not maintainable to have a massive list of file exports in the package.json as the exports field would need to be carefully edited whenever a file is added, renamed or removed. Often there’s no feedback the exports are wrong due to an oversight or typo until the package is published and users complain something broke.

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 private) that is explicitly blacklisted via the package exports field.

IMO the most reasonable way to deal with the dual package hazard is to publish for each piece of the API a single CJS / .js file with module.exports =. Then have an index.mjs and index.js file that exports them as a collection, with the index files exposed via conditional exports in the package.json exports field.

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 package.json fields (files, main, module, exports) and file structure that is not overly complex would go a long way.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
MylesBorins added 2 commits April 29, 2020 10:59
@MylesBorins
Update esm.md
d059abf
@MylesBorins
fixup: lots of changes
3785420
@addaleax addaleax mentioned this pull request May 3, 2020
Fix package.json exports for ts-node/register TypeStrong/ts-node#1026
Closed
robpalme
robpalme reviewed May 4, 2020
View reviewed changes
Comment thread doc/api/esm.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread doc/api/esm.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread doc/api/esm.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
MylesBorins and others added 5 commits May 4, 2020 11:03
@MylesBorins @robpalme
Update doc/api/esm.md
991af5d 
Co-authored-by: Rob Palmer <[email protected]>
@MylesBorins @robpalme
Update doc/api/esm.md
33cdd05 
Co-authored-by: Rob Palmer <[email protected]>
@MylesBorins
fixup: remove note regarding tooling
a313dbc
@MylesBorins
fixup: explicitly enumerate
68a09d5
@MylesBorins
fixup: include folder example
65cdd69
@MylesBorins

MylesBorins commented May 4, 2020

Copy link
Copy Markdown
Contributor Author

@guybedford I've made some changes and would like another review from you please
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
hybrist
hybrist approved these changes May 4, 2020
View reviewed changes
Comment thread doc/api/esm.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
MylesBorins added 2 commits May 4, 2020 11:33
@MylesBorins
fixup: be explicit about other limitations of ./ export
a1614cc
@MylesBorins
fixup: fix lint
bab451c
robpalme
robpalme approved these changes May 4, 2020
View reviewed changes
GeoffreyBooth
GeoffreyBooth approved these changes May 4, 2020
View reviewed changes

@GeoffreyBooth GeoffreyBooth left a comment

Copy link
Copy Markdown
Member

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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@MylesBorins

MylesBorins commented May 4, 2020

Copy link
Copy Markdown
Contributor Author

I'll land this in 24 hours unless anyone objects
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@MylesBorins

MylesBorins commented May 4, 2020 via email

Copy link
Copy Markdown
Contributor Author
@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> .
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
guybedford
guybedford reviewed May 5, 2020
View reviewed changes
Comment thread doc/api/esm.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
@guybedford

guybedford commented May 5, 2020

Copy link
Copy Markdown
Contributor

Thanks @MylesBorins, I've added the suggestion with some slight rewording. @GeoffreyBooth let me know if that works for you?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
MylesBorins and others added 2 commits May 5, 2020 01:26
@MylesBorins @guybedford
Update doc/api/esm.md
4f97c02 
Co-authored-by: Guy Bedford <[email protected]>
@MylesBorins
fixup: one more doc tweak
b15fc40
@MylesBorins

MylesBorins commented May 5, 2020

Copy link
Copy Markdown
Contributor Author

made some more changes PTAL
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
addaleax
addaleax approved these changes May 5, 2020
View reviewed changes
MylesBorins added a commit that referenced this pull request May 5, 2020
@MylesBorins
doc: explicitly doc package.exports is breaking
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 <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorins

MylesBorins commented May 5, 2020

Copy link
Copy Markdown
Contributor Author

landed in 1ffd182
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@MylesBorins MylesBorins closed this May 5, 2020
codebytere pushed a commit that referenced this pull request May 7, 2020
@MylesBorins @codebytere
doc: explicitly doc package.exports is breaking
07372e9 
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 <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@codebytere codebytere mentioned this pull request May 18, 2020
v14.3.0 proposal #33452
Merged
codebytere pushed a commit that referenced this pull request Jun 7, 2020
@MylesBorins @codebytere
doc: explicitly doc package.exports is breaking
c6cf048 
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 <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@codebytere codebytere mentioned this pull request Jun 9, 2020
v12.18.1 proposal #33811
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ljharb ljharb ljharb left review comments
@hybrist hybrist hybrist approved these changes
@guybedford guybedford guybedford approved these changes
@GeoffreyBooth GeoffreyBooth GeoffreyBooth approved these changes
@addaleax addaleax addaleax approved these changes
+5 more reviewers
@novemberborn novemberborn novemberborn left review comments
@eemeli eemeli eemeli left review comments
@jaydenseric jaydenseric jaydenseric left review comments
@targos targos targos left review comments
@robpalme robpalme robpalme approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

doc Issues and PRs related to Node.js documentation. esm Issues and PRs related to the ECMAScript Modules implementation.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

12 participants

@MylesBorins @guybedford @jaydenseric @novemberborn @ljharb @GeoffreyBooth @hybrist @eemeli @addaleax @targos @robpalme @nodejs-github-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.