-
-
Notifications
You must be signed in to change notification settings - Fork 31.9k
doc: update and improve the CommonJS page #57055
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: update and improve the CommonJS page #57055
Conversation
Review requested:
|
c9a9574
to
bf9e61c
Compare
2. Else, load X/index.js as an CommonJS module. STOP. | ||
2. If X/index.json is a file, parse X/index.json to a JavaScript object. STOP | ||
3. If X/index.node is a file, load X/index.node as binary addon. STOP | ||
MAYBE_DETECT_AND_LOAD(X) |
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.
* <a id="modules_new_sourcemap_payload" href="module.html#new-sourcemappayload">`new SourceMap(payload)`</a> | ||
* <a id="modules_sourcemap_payload" href="module.html#sourcemappayload">`sourceMap.payload`</a> | ||
* <a id="modules_sourcemap_findentry_linenumber_columnnumber" href="module.html#sourcemapfindentrylinenumber-columnnumber">`sourceMap.findEntry(lineNumber, columnNumber)`</a> |
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.
I've aligned these with the rest of the anchor tags, but I don't think that these (and the ones above) are correct.
This comment makes me think that these should not be visible:
Line 1268 in cc7018e
<!-- Anchors to make sure old links find a target --> |
And indeed they (to me at least) look totally out of place in the rendered page:
@aduh95 from the git history it seems like you placed these here (#34747), could you advice whether these are ok to stay, be hidden or removed? (or converted to i
tags and moved in the various sections of this page (like here))
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.
PS: also the new SourceMap(payload)
and sourceMap.findEntry(lineNumber, columnNumber)
links are actually broken
These changes: - update outdated info (`module.buildinModules` includes all built-ins) - fix various typos - improve consistency - polish and make more clear some sentences
bf9e61c
to
b3c1f55
Compare
This is very hard to review, and will be a nightmare to backport. Could you send several PRs, each focusing on a specific point, please? |
oh... ok 🥲👍 |
It's too cumbersome to move everything to simple PRs... I think I will then drop a bunch of improvements here 😓 (I don't think it makes sense to open a hundred small/trivial PRs for updating a single page) |
These changes:
module.buildinModules
includes all built-ins since 23.5.0)Fixes #51780