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 documentation consolidation and reordering #36046

Closed
wants to merge 14 commits into from

Conversation

guybedford
Copy link
Contributor

@guybedford guybedford commented Nov 9, 2020

I just went through the esm.md docs and consolidated sections where it seemed to make sense as well as reordering the structure slightly now that the layout of this page is finally stable.

Changes include:

  • Removing explicit "experimental" from headings, and instead setting the > Stability note.
  • Removed the distinction of deep package specifiers for a single bare specifier definition. This is because with exports, imports etc deep package specifiers are in theory only one of many types of bare specifier at this point.
  • Added a note that file extensions are necessary for abslute and relative specifiers
  • Added a note that file extensions are optional for bare specifiers
  • Moved up the "mandatory file extensions" section into the import specifiers section.
  • Created a new "URLs" section as a subsection of import specifiers, and included sections on file:, data: and node: URLs. Brought all the file resolution considerations under the "file: Imports" heading here.
  • Added a note about pathToFileURL
  • Updated the note that absolute specifiers can start with / or //
  • Split import.meta into import.meta.url and import.meta.resolve subsections. Expanded on their use cases.
  • Moved up the dynamic import() expressions section
  • Moved "Differences between CommonJS and ES modules" to the bottom of the "Interoperability with CommonJS" section.
  • Simplified the "Interoperability with CommonJS" section to remove repetition / information that repeats directly what is already said above about specifier resolution, and also to get straight to the point of what is supported.
  • Added a note that require of ES modules is not supported because ES modules are asynchronous.
  • Move dynamic import before import meta
  • Move import before require in interop section
  • Separate "No require, exports, module.exports" from "No __dirname, __filename"
  • Move CommonJS, JSON, native modules to "differences" section, with full practical examples for the replacements of each
  • Update process.dlopen documentation to be slightly more ES modules use case appropriate.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/modules

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Nov 9, 2020
doc/api/cli.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 Outdated Show resolved Hide resolved
doc/api/esm.md Outdated Show resolved Hide resolved
doc/api/esm.md Outdated Show resolved Hide resolved
@aduh95 aduh95 added the esm Issues and PRs related to the ECMAScript Modules implementation. label Nov 9, 2020
doc/api/cli.md Show resolved Hide resolved
Copy link
Member

@bmeck bmeck left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM. While we're at it, I wouldn't mind a switch from builtin to built-in but that may be a "tabs vs. spaces" kind of discussion, so I'm OK with this as it is.

Copy link
Member

@GeoffreyBooth GeoffreyBooth left a comment

Choose a reason for hiding this comment

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

Thank you for doing this!

@guybedford
Copy link
Contributor Author

I know we were nearly ready to go on this, but I just took a look today and ended up putting a little more time into reworking the differences section. See c4258d2 for the latest commit.

The gist of the new changes are the last bullet points in the description of this issue:

  • Move dynamic import before import meta
  • Move import before require in interop section
  • Separate "No require, exports, module.exports" from "No __dirname, __filename"
  • Move CommonJS, JSON, native modules to "differences" section, with full practical examples for the replacements of each

@guybedford
Copy link
Contributor Author

@devsnek I've posted an update to remove the example, and mentioned createRequire first in this case.

doc/api/esm.md Outdated Show resolved Hide resolved
@guybedford
Copy link
Contributor Author

With the new link to process.dlopen I've also now made some minor edits to the process.dlopen documentation to make it slightly more accessible to ES module consumers - noting that ES modules are a valid exception for usage, removing require.resolve from the example, and including the explicit module object creation there. @Trott if you have a moment to review the latest commit here I'd value your sign-off here again.

doc/api/esm.md Outdated Show resolved Hide resolved
doc/api/process.md Outdated Show resolved Hide resolved
doc/api/process.md Outdated Show resolved Hide resolved
guybedford added a commit that referenced this pull request Nov 15, 2020
PR-URL: #36046
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
@guybedford
Copy link
Contributor Author

Landed in 187ce5b.

@guybedford guybedford closed this Nov 15, 2020
codebytere pushed a commit that referenced this pull request Nov 22, 2020
PR-URL: #36046
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
@codebytere codebytere mentioned this pull request Nov 22, 2020
BethGriggs pushed a commit that referenced this pull request Dec 9, 2020
PR-URL: #36046
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
PR-URL: #36046
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Dec 10, 2020
BethGriggs pushed a commit that referenced this pull request Dec 15, 2020
PR-URL: #36046
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
@broofa
Copy link

broofa commented Dec 16, 2020

This PR removed the [very helpful] example for how to reproduce the __dirname and __filename vars. Was this intentional, or is this a a regression that should be fixed? See diff here.)

See also, #28114 and #28282

@guybedford
Copy link
Contributor Author

@broofa all the examples intentionally use the new URL('./specifier', import.meta.url) pattern because this makes asset reference analysis in bundlers much simpler, than the complex __filename and __dirname analysis that will be further complicated by techniques to set them, such as for example is used in ncc when relocating assets.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. esm Issues and PRs related to the ECMAScript Modules implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants