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.

Sign up for GitHub

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

Jump to bottom

doc: esm: improve dual package hazard docs #30345

Closed
wants to merge 9 commits into from
Closed

doc: esm: improve dual package hazard docs #30345

wants to merge 9 commits into from

Conversation

GeoffreyBooth
Copy link
Member

@GeoffreyBooth GeoffreyBooth commented Nov 9, 2019 •
edited
Loading

This updates the ES modules docs regarding hazards related to dual packages per the findings in nodejs/modules#409. Basically, the hazard that we previously thought was isolated to divergent specifiers (e.g. 'pkg' resolving to index.cjs for CommonJS and index.mjs for ESM) happens for all dual packages even if the two entry points are at different specifiers (e.g. 'pkg' and 'pkg/module'). Therefore the section on writing dual packages applies not just to conditional exports but to the ES modules implementation generally.

Follow up to #30051.

cc @guybedford @MylesBorins @nodejs/modules-active-members

Checklist

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Nov 9, 2019
guybedford
guybedford reviewed Nov 10, 2019
View reviewed changes
doc/api/esm.md Outdated Show resolved Hide resolved
guybedford
guybedford reviewed Nov 11, 2019
View reviewed changes
doc/api/esm.md Outdated Show resolved Hide resolved
@guybedford
Copy link
Contributor

What I'm looking for specifically is a note like -

Note: While --experimental-conditional-exports is flagged, publishing a package with this pattern will break when used with require() in modern Node.js, unless the --experimental-conditional-exports flag is used by all package consumers.

@guybedford
Copy link
Contributor

The problem specifically is that the break is not visible in testing, and will only become obvious much further down the line.

@GeoffreyBooth GeoffreyBooth force-pushed the dual-package-docs branch 3 times, most recently from 0ba8715 to 68ab812 Compare November 12, 2019 22:04
guybedford
guybedford reviewed Nov 12, 2019
View reviewed changes
doc/api/esm.md Outdated Show resolved Hide resolved
robpalme
robpalme reviewed Nov 13, 2019
View reviewed changes
Copy link

@robpalme robpalme left a comment

Choose a reason for hiding this comment

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

Minor wording suggestions.

doc/api/esm.md Outdated Show resolved Hide resolved
doc/api/esm.md Outdated Show resolved Hide resolved
guybedford
guybedford reviewed Nov 14, 2019
View reviewed changes
doc/api/esm.md Outdated Show resolved Hide resolved
@guybedford guybedford added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 14, 2019
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/26600/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/26623/

vsemozhetbyt
vsemozhetbyt reviewed Nov 16, 2019
View reviewed changes
doc/api/esm.md Outdated Show resolved Hide resolved
vsemozhetbyt
vsemozhetbyt reviewed Nov 16, 2019
View reviewed changes
doc/api/esm.md Outdated Show resolved Hide resolved
GeoffreyBooth and others added 2 commits November 16, 2019 11:50
@GeoffreyBooth @vsemozhetbyt
doc: typo
94e85ab 
Co-Authored-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
@GeoffreyBooth @vsemozhetbyt
doc: typo
912f281 
Co-Authored-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 17, 2019
@GeoffreyBooth @vsemozhetbyt @MylesBorins
doc: esm: improve dual package hazard docs
d9efc7d 
Co-Authored-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
PR-URL: #30345
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
@MylesBorins
Copy link
Contributor

landed in d9efc7d

@GeoffreyBooth GeoffreyBooth deleted the dual-package-docs branch November 17, 2019 07:55
MylesBorins pushed a commit that referenced this pull request Nov 17, 2019
@GeoffreyBooth @vsemozhetbyt @MylesBorins
doc: esm: improve dual package hazard docs
b590533 
Co-Authored-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
PR-URL: #30345
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Nov 19, 2019
Merged
MylesBorins pushed a commit that referenced this pull request Jan 12, 2020
@GeoffreyBooth @vsemozhetbyt @MylesBorins
doc: esm: improve dual package hazard docs
b99eca9 
Co-Authored-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
PR-URL: #30345
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
@targos targos mentioned this pull request Jan 15, 2020
Closed
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
@GeoffreyBooth @vsemozhetbyt @BethGriggs
doc: esm: improve dual package hazard docs
138a905 
Co-Authored-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
PR-URL: #30345
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vsemozhetbyt vsemozhetbyt vsemozhetbyt left review comments

@robpalme robpalme robpalme left review comments

@guybedford guybedford guybedford approved these changes

@jkrems jkrems jkrems approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@GeoffreyBooth @guybedford @nodejs-github-bot @MylesBorins @jkrems @robpalme @vsemozhetbyt