★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/30345Nouvelle recherche | Portail wanayoo
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: esm: improve dual package hazard docs #30345

Closed
wants to merge 9 commits into from

Conversation

@GeoffreyBooth
Copy link
Member

GeoffreyBooth commented Nov 9, 2019 •

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
doc/api/esm.md Outdated Show resolved Hide resolved
doc/api/esm.md Show resolved Hide resolved
doc/api/esm.md Show resolved Hide resolved
doc/api/esm.md Show resolved Hide resolved
doc/api/esm.md Outdated Show resolved Hide resolved
doc/api/esm.md Show resolved Hide resolved
doc/api/esm.md Show resolved Hide resolved
@GeoffreyBooth GeoffreyBooth force-pushed the GeoffreyBooth:dual-package-docs branch Nov 12, 2019
@guybedford

This comment has been minimized.

Copy link
Contributor

guybedford commented Nov 12, 2019

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

This comment has been minimized.

Copy link
Contributor

guybedford commented Nov 12, 2019

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 GeoffreyBooth:dual-package-docs branch 3 times, most recently Nov 12, 2019
doc/api/esm.md Show resolved Hide resolved
doc/api/esm.md Outdated Show resolved Hide resolved
@GeoffreyBooth GeoffreyBooth force-pushed the GeoffreyBooth:dual-package-docs branch to 1f7b577 Nov 12, 2019
@jkrems
jkrems approved these changes Nov 12, 2019
Copy link

robpalme left a comment

Minor wording suggestions.

doc/api/esm.md Outdated Show resolved Hide resolved
doc/api/esm.md Outdated Show resolved Hide resolved
@GeoffreyBooth GeoffreyBooth force-pushed the GeoffreyBooth:dual-package-docs branch Nov 14, 2019
doc/api/esm.md Show resolved Hide resolved
doc/api/esm.md Outdated Show resolved Hide resolved
doc/api/esm.md Show resolved Hide resolved
doc/api/esm.md Show resolved Hide resolved
@GeoffreyBooth GeoffreyBooth force-pushed the GeoffreyBooth:dual-package-docs branch to 4b75b64 Nov 14, 2019
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

doc/api/esm.md Outdated Show resolved Hide resolved
doc/api/esm.md Outdated Show resolved Hide resolved
GeoffreyBooth and others added 2 commits Nov 16, 2019
doc: typo
Co-Authored-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
doc: typo
Co-Authored-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
MylesBorins added a commit that referenced this pull request Nov 17, 2019
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

This comment has been minimized.

Copy link
Member

MylesBorins commented Nov 17, 2019

landed in d9efc7d

@GeoffreyBooth GeoffreyBooth deleted the GeoffreyBooth:dual-package-docs branch Nov 17, 2019
MylesBorins added a commit that referenced this pull request Nov 17, 2019
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
MylesBorins added a commit that referenced this pull request Jan 12, 2020
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>
BethGriggs added a commit that referenced this pull request Feb 6, 2020
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
saitonakamura pushed a commit to saitonakamura/node that referenced this pull request Apr 7, 2020
Co-Authored-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
PR-URL: nodejs#30345
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.