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: Extend esm examples, document and lint import forms for process + Buffer #39043

Closed
wants to merge 9 commits into from

Conversation

guybedford
Copy link
Contributor

@guybedford guybedford commented Jun 15, 2021

This updates the use of esm examples in the documentation to cover more core modules while encouraging the use of Buffer and process imports / requires where possible, instead of saying that this is unnecessary. It does not deprecate or say that the global forms are deprecated though.

Summary:

  • Split examples into ESM / CJS sections for: async_context, async_hooks, cluster, dgram, diagnostics_channel (as far as I could go in one sitting!)
  • Update both process and buffer to always show the import form in usage examples, noting this is encouraged but not required.
  • In addition, destructured import forms are given preference where suitable.
  • Add an eslint rule for the docs to not include process / Buffer without being imported and fix all cases.

@nodejs/modules

@github-actions github-actions bot added the doc Issues and PRs related to the documentations. label Jun 15, 2021
doc/api/async_hooks.md Outdated Show resolved Hide resolved
doc/api/async_hooks.md Show resolved Hide resolved
doc/api/async_hooks.md Show resolved Hide resolved
doc/api/async_hooks.md Outdated Show resolved Hide resolved
Co-authored-by: Antoine du Hamel <[email protected]>
doc/api/async_context.md Outdated Show resolved Hide resolved
Co-authored-by: Bradley Farias <[email protected]>
@bmeck
Copy link
Member

bmeck commented Jun 15, 2021

It might make sense to modify

node/.eslintrc.js

Lines 65 to 101 in f4609bd

files: ['**/*.md/*.cjs', '**/*.md/*.js'],
parserOptions: {
sourceType: 'script',
ecmaFeatures: { impliedStrict: true }
},
rules: { strict: 'off' },
},
{
files: [
'**/*.md/*.mjs',
'doc/api/esm.md/*.js',
'doc/api/packages.md/*.js',
],
parserOptions: { sourceType: 'module' },
rules: { 'no-restricted-globals': [
'error',
{
name: '__filename',
message: 'Use import.meta.url instead',
},
{
name: '__dirname',
message: 'Not available in ESM',
},
{
name: 'exports',
message: 'Not available in ESM',
},
{
name: 'module',
message: 'Not available in ESM',
},
{
name: 'require',
message: 'Use import instead',
},
] },
and add a no-restricted globals for Buffer and process.

@guybedford
Copy link
Contributor Author

Thanks, I've gone ahead and added the lint rule for Buffer and process and fixed up all cases.

I've also updated the process docs to use destructured import forms wherever possible as well eg import { cwd } from 'process'.

aduh95
aduh95 previously requested changes Jun 15, 2021
doc/api/crypto.md Outdated Show resolved Hide resolved
doc/api/async_hooks.md Outdated Show resolved Hide resolved
const module = { exports: {} };
process.dlopen(module, path.join(__dirname, 'local.node'),
os.constants.dlopen.RTLD_NOW);
dlopen(module, join(__dirname, 'local.node'), constants.dlopen.RTLD_NOW);
Copy link
Contributor

Choose a reason for hiding this comment

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

I subjectively prefer path.join rather than simply join, feel free to disregard if you disagree.

Suggested change
dlopen(module, join(__dirname, 'local.node'), constants.dlopen.RTLD_NOW);
dlopen(module, path.join(__dirname, 'local.node'), constants.dlopen.RTLD_NOW);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did go quite agressive on the destructured imports, down to leaving out cases like "on". I understand some of these get tricky, but want to try and encourage this as much as possible for tree-shaking style optimizations. In theory leaving out the member expression access is more performant as well although it's a micro-optimization for sure. Will leave this in unless anyone wants to push back further.

@aduh95 aduh95 dismissed their stale review June 15, 2021 21:40

Addressed

Co-authored-by: Antoine du Hamel <[email protected]>
@guybedford guybedford changed the title doc: Extend esm examples, including documenting import forms for process + Buffer doc: Extend esm examples, document and lint import forms for process + Buffer Jun 15, 2021
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, docs only change, would want to do some styling work to agree how to reduce noise viewing overall size of docs in a different PR.

guybedford added a commit that referenced this pull request Jun 25, 2021
@guybedford
Copy link
Contributor Author

Landed in f4d0a6a.

@guybedford guybedford closed this Jun 25, 2021
@guybedford guybedford deleted the docs-esm-examples branch June 25, 2021 18:29
@aduh95
Copy link
Contributor

aduh95 commented Jul 1, 2021

@guybedford the lint rule only targets ESM syntax, is that on purpose? For info there are 135 occurrences of Buffer and process used as globals in CJS code snippets.

targos pushed a commit that referenced this pull request Jul 11, 2021
tniessen added a commit to tniessen/node that referenced this pull request Mar 18, 2022
nodejs-github-bot pushed a commit that referenced this pull request Mar 21, 2022
Refs: #39043

PR-URL: #42394
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Akhil Marsonya <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
juanarbol pushed a commit that referenced this pull request Apr 4, 2022
Refs: #39043

PR-URL: #42394
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Akhil Marsonya <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
juanarbol pushed a commit to juanarbol/node that referenced this pull request Apr 5, 2022
Refs: nodejs#39043

PR-URL: nodejs#42394
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Akhil Marsonya <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
juanarbol pushed a commit that referenced this pull request Apr 6, 2022
Refs: #39043

PR-URL: #42394
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Akhil Marsonya <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
xtx1130 pushed a commit to xtx1130/node that referenced this pull request Apr 25, 2022
Refs: nodejs#39043

PR-URL: nodejs#42394
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Akhil Marsonya <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
juanarbol pushed a commit that referenced this pull request May 31, 2022
Refs: #39043

PR-URL: #42394
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Akhil Marsonya <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
danielleadams pushed a commit that referenced this pull request Jun 27, 2022
Refs: #39043

PR-URL: #42394
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Akhil Marsonya <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
targos pushed a commit that referenced this pull request Jul 11, 2022
Refs: #39043

PR-URL: #42394
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Akhil Marsonya <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
targos pushed a commit that referenced this pull request Jul 31, 2022
Refs: #39043

PR-URL: #42394
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Akhil Marsonya <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
Refs: nodejs/node#39043

PR-URL: nodejs/node#42394
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Akhil Marsonya <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants