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

require node: prefixes for Node.js builtins #1080

Merged
merged 2 commits into from Mar 12, 2024

Conversation

boneskull
Copy link
Contributor

@boneskull boneskull commented Mar 8, 2024

This PR adds a plugin (eslint-plugin-node-import) which provides a rule to require the node: prefix when import-ing or require-ing Node.js builtins.

node:-prefixed builtins better illustrate intent and reduce the risk of erroneously pulling in 3rd-party packages.

The second commit updates the codebase (done via eslint --fix) to use the node: prefix where appropriate:

Two places were special cases:

  1. In viz, old Webpack does not understand the node: prefix; this had to be exempted from the rule.
  2. In tofu, I added another test to assert both prefixed and non-prefixed builtin references work as expected.

Note that test fixture projects are ignored by ESLint; they have not been changed.


  • chore(eslint): require prefixed node builtins
  • chore: use prefixed node builtins where appropriate

@boneskull boneskull requested review from legobeat and a team as code owners March 8, 2024 20:40
@github-actions github-actions bot added pkg:lavamoat-viz Changes in package lavamoat-viz pkg:lavamoat-perf Changes in package lavamoat-perf pkg:lavamoat-core Changes in package lavamoat-core pkg:@lavamoat/aa Changes in package @lavamoat/aa pkg:@lavamoat/allow-scripts Changes in package @lavamoat/allow-scripts pkg:lavamoat-browserify Changes in package lavamoat-browserify pkg:lavamoat-tofu Changes in package lavamoat-tofu pkg:@lavamoat/webpack Changes in package @lavamoat/webpack pkg:@lavamoat/lavapack Changes in package @lavamoat/lavapack pkg:survey Changes in package survey labels Mar 8, 2024
@boneskull boneskull force-pushed the boneskull/require-builtins-eslint branch from cbca721 to 07bf941 Compare March 8, 2024 20:46
@boneskull
Copy link
Contributor Author

I made this because I don't want to feel compelled to add suggestions on other people's PRs to change it. 😄

@boneskull boneskull force-pushed the boneskull/require-builtins-eslint branch 3 times, most recently from 5c41b7b to c7b3a89 Compare March 8, 2024 22:06
@boneskull
Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @boneskull and the rest of your teammates on Graphite Graphite

@boneskull boneskull force-pushed the boneskull/require-builtins-eslint branch from c7b3a89 to 734ab44 Compare March 11, 2024 23:57
Copy link
Contributor

@legobeat legobeat left a comment

Choose a reason for hiding this comment

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

lgtm

@boneskull boneskull force-pushed the boneskull/require-builtins-eslint branch from 734ab44 to 13fe60c Compare March 12, 2024 20:13
This adds a rule which requires all `require()`s or `import`s use the `node:` prefix when requesting builtins.

`node:`-prefixed builtins better illustrate intent and reduce the risk of erroneously pulling in 3rd-party packges.
This change adds the `node:` prefix to (almost) all `require()` calls of Node.js builtins.

Two places were special cases:

1. In `viz`, old Webpack does not understand the `node:` prefix; this had to be exempted from the rule.
2. In `tofu`, I added another test to assert both prefixed and non-prefixed builtin references work as expected.

Note that test fixture projects are ignored by ESLint; they have not been changed.
@boneskull boneskull force-pushed the boneskull/require-builtins-eslint branch from 13fe60c to 5da2c26 Compare March 12, 2024 21:26
@boneskull boneskull merged commit ba0975a into main Mar 12, 2024
25 checks passed
@boneskull boneskull deleted the boneskull/require-builtins-eslint branch March 12, 2024 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:@lavamoat/aa Changes in package @lavamoat/aa pkg:@lavamoat/allow-scripts Changes in package @lavamoat/allow-scripts pkg:@lavamoat/lavapack Changes in package @lavamoat/lavapack pkg:@lavamoat/webpack Changes in package @lavamoat/webpack pkg:lavamoat-browserify Changes in package lavamoat-browserify pkg:lavamoat-core Changes in package lavamoat-core pkg:lavamoat-perf Changes in package lavamoat-perf pkg:lavamoat-tofu Changes in package lavamoat-tofu pkg:lavamoat-viz Changes in package lavamoat-viz pkg:survey Changes in package survey
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants