FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

doc: ESM documentation consolidation and reordering by guybedford · Pull Request #36046 · nodejs/node · GitHub

/ node Public

doc: ESM documentation consolidation and reordering - #36046

Closed
guybedford wants to merge 14 commits into
nodejs:masterfrom
guybedford:docs-consolidation
ConversationCommits (14)ChecksFiles changed
Closed

doc: ESM documentation consolidation and reordering#36046
guybedford wants to merge 14 commits into
nodejs:masterfrom
guybedford:docs-consolidation

Conversation

guybedford commented Nov 9, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 commented Nov 9, 2020

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/modules

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Nov 9, 2020
ljharb reviewed Nov 9, 2020
View reviewed changes
Comment thread doc/api/cli.md
Comment thread doc/api/esm.md
aduh95 reviewed Nov 9, 2020
View reviewed changes
Comment thread doc/api/esm.md Outdated
Comment thread doc/api/esm.md Outdated
Comment thread doc/api/esm.md Outdated
guybedford and others added 3 commits November 8, 2020 16:39
aduh95 reviewed Nov 9, 2020
View reviewed changes
Comment thread doc/api/esm.md Outdated
guybedford commented Nov 9, 2020
View reviewed changes
Comment thread doc/api/esm.md
aduh95 added the esm Issues and PRs related to the ECMAScript Modules implementation. label Nov 9, 2020
bmeck reviewed Nov 9, 2020
View reviewed changes
Comment thread doc/api/cli.md
bmeck approved these changes Nov 9, 2020
View reviewed changes

bmeck left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

Trott approved these changes Nov 9, 2020
View reviewed changes

Trott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

GeoffreyBooth approved these changes Nov 11, 2020
View reviewed changes

GeoffreyBooth left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thank you for doing this!

guybedford commented Nov 11, 2020

Copy link
Copy Markdown
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 commented Nov 12, 2020

Copy link
Copy Markdown
Contributor Author

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

aduh95 reviewed Nov 12, 2020
View reviewed changes
Comment thread doc/api/esm.md Outdated

guybedford commented Nov 13, 2020

Copy link
Copy Markdown
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.

Trott reviewed Nov 14, 2020
View reviewed changes
Comment thread doc/api/esm.md Outdated
Trott reviewed Nov 14, 2020
View reviewed changes
Comment thread doc/api/process.md Outdated
Trott reviewed Nov 14, 2020
View reviewed changes
Comment thread doc/api/process.md Outdated
Apply suggestions from code review
f519339 
Co-authored-by: Rich Trott <rtrott@gmail.com>
guybedford added a commit that referenced this pull request Nov 15, 2020
doc: esm docs consolidation and reordering
187ce5b 
PR-URL: #36046
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>

guybedford commented Nov 15, 2020

Copy link
Copy Markdown
Contributor Author

Landed in 187ce5b.

guybedford closed this Nov 15, 2020
codebytere pushed a commit that referenced this pull request Nov 22, 2020
doc: esm docs consolidation and reordering
0ff384b 
PR-URL: #36046
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>
codebytere mentioned this pull request Nov 22, 2020
Merged
BethGriggs pushed a commit that referenced this pull request Dec 9, 2020
doc: esm docs consolidation and reordering
a967baf 
PR-URL: #36046
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
doc: esm docs consolidation and reordering
a6937e9 
PR-URL: #36046
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>
BethGriggs mentioned this pull request Dec 10, 2020
Merged
BethGriggs pushed a commit that referenced this pull request Dec 15, 2020
doc: esm docs consolidation and reordering
800e1db 
PR-URL: #36046
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>

broofa commented Dec 16, 2020

Copy link
Copy Markdown

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 commented Dec 16, 2020

Copy link
Copy Markdown
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.

aduh95 mentioned this pull request May 16, 2021
Closed
aduh95 mentioned this pull request Aug 2, 2023
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

ljharb ljharb left review comments
aduh95 aduh95 approved these changes
GeoffreyBooth GeoffreyBooth approved these changes
Trott Trott approved these changes
+1 more reviewer
bmeck bmeck approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

doc Issues and PRs related to the documentations. 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.

10 participants

Back | FazBrowse Home | New Git URL