feat: added prefer-node-builtin-imports rule

[GoldStrikeArch]

Fix to this issue Added a new rule prefer-node-builtin-imports. All test cases are taken from eslint-plugin-unicorn_test

[ljharb]

@GoldStrikeArch are you still interested in completing this PR?

[GoldStrikeArch]

@GoldStrikeArch are you still interested in completing this PR?

Yeah, forgot about this. I will try to finish it on this weekend

[sevenc-nanashi]

Any updates on this PR? I'm very excited about this rule.

(I'd like to take over this PR if needed)

[ljharb]

@sevenc-nanashi if you'd like to do so, please do NOT make a new PR - instead, comment here with a link to your branch containing this PR, rebased and updated per https://github.com/import-js/eslint-plugin-import/pull/3024#discussion_r1665739477, and i'll pull in the changes.

[sevenc-nanashi]

@ljharb I rebased and added some changes according to the review: https://github.com/sevenc-nanashi/eslint-plugin-import/tree/feat/prefer-node-protocol?tab=readme-ov-file

• This rule now accepts always or never option.
• Removed this rule from recommended option.
• Fixed tests.

I think the name should be prefer-node-protocol, as the name prefer-node-builtin-imports sounds like preferring using node's builtin over other packages.

[ljharb]

@sevenc-nanashi i don't think it should have "prefer" in it at all, because the rule won't be expressing an opinion that using, or avoiding, the protocol is preferred, but I agree that "node protocol" is better wording.

Perhaps enforce-node-protocol-usage?

[sevenc-nanashi]

@ljharb I renamed to enforce-node-protocol-usage.

[ljharb]

The rule should either default to “never” - the far more universal behavior - or have no default at all.

The docs will need a lot of work too - deno and hun only support the protocol, i believe, so you’d want to set the rule to “always”; if you’re supporting a node version, bundler, or tool that doesn’t understand the node protocol, you’d want to set it to “never”.

Additionally, prefix-only core modules, like node:test, node:sea, node:sqlite, etc need to remain unaffected by the “never” setting. Although, because module.builtinModules doesn’t include prefix-only modules, you may not need to do anything here but add test cases.

[sevenc-nanashi]

@ljharb I made these changes:

• The rule will throw error when the option is not specified
• Added test cases that require("node:test") is valid in 'never'

[ljharb]

k, i've pulled in and rebased the changes. I also made it use is-core-module since module.builtinModules isn't available in older node versions.

[ljharb]

@sevenc-nanashi i haven't reviewed the tests yet; assuming that every module being tested is tested with both configurations, and that eg node:test and node:sea are tested, and that tests pass, then this should be good.

[sevenc-nanashi]

@ljharb I made some changes:

• Use object-based option schema to create mandatory option
• Added some test cases

[ljharb]

hmm, lots of the tests are failing for me locally, but i'm not sure why. the logic seems correct.

[sevenc-nanashi]

@ljharb Looks like the issue caused by the guard of visitor.

The fix

diff --git a/src/rules/enforce-node-protocol-usage.js b/src/rules/enforce-node-protocol-usage.js
index f8f3ee39..548f303b 100644
--- a/src/rules/enforce-node-protocol-usage.js
+++ b/src/rules/enforce-node-protocol-usage.js
@@ -96,11 +96,16 @@ module.exports = {
       url: docsUrl('enforce-node-protocol-usage'),
     },
     fixable: 'code',
-    schema: [
-      {
-        enum: ['always', 'never'],
-      },
-    ],
+    schema: {
+      type: 'array',
+      minItems: 1,
+      maxItems: 1,
+      items: [
+        {
+          enum: ['always', 'never'],
+        },
+      ],
+    },
     messages,
   },
   create(context) {
@@ -115,18 +120,12 @@ module.exports = {
         return checkAndReport(arg, context);
       },
       ExportNamedDeclaration(node) {
-        if (!isStringLiteral(node)) { return; }
-
         return checkAndReport(node.source, context);
       },
       ImportDeclaration(node) {
-        if (!isStringLiteral(node)) { return; }
-
         return checkAndReport(node.source, context);
       },
       ImportExpression(node) {
-        if (!isStringLiteral(node)) { return; }
-
         return checkAndReport(node.source, context);
       },
     };

In my environment, all tests passed with this patch.

[sevenc-nanashi]

btw: is it actually worth supporting Node v4?

• No one uses that,
• It's dangerous not to upgrade Node,
• It makes CI slower,
• It makes installation slower and node_modules heavier, (because of polyfills)
• It makes development slower, (I saw spread expression was replaced with [].concat; there would be less code changes for backward compatibility like this)
• And users can choose not to upgrade eslint-plugin-import (There should be a major version change though)

[ljharb]

@sevenc-nanashi yes, people do; it's not dangerous in many contexts; i don't care about CI speed; disk space is infinite and free and npm installation is very fast; the [].concat is something i'd do in any codebase, spread is slow and should forever be avoided; doing a major version bump like that means that users on the old major are stuck without features, bugfixes, and security patches, so it's actually the most harmful thing any package can do for the ecosystem.

[ljharb]

So one thing to note: the list of core modules is dependent on the node version that eslint is running in.

I think perhaps it might make sense to add a settings option for "node version" that this rule uses, that will override the current eslint node version (isCoreModule has that API already). Thoughts?

[ljharb]

ok, updated. lmk what yall think

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 86.27451% with 7 lines in your changes missing coverage. Please review.

Project coverage is 95.10%. Comparing base (d5f2950) to head (8c8058c). Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/rules/enforce-node-protocol-usage.js	86.27%	7 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3024      +/-   ##
==========================================
+ Coverage   94.72%   95.10%   +0.37%     
==========================================
  Files          83       84       +1     
  Lines        3583     3634      +51     
  Branches     1252     1279      +27     
==========================================
+ Hits         3394     3456      +62     
+ Misses        189      178      -11     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[jozefizso]

When this rule will be released?

[JounQin]

When this rule will be released?

Hi, @jozefizso, may I ask is there anything breaking you to use n/prefer-node-protocol instead?

[gshpychka]

When this rule will be released?

Hi, @jozefizso, may I ask is there anything breaking you to use n/prefer-node-protocol instead?

This is off-topic for this PR, no?

[JounQin]

Just curious.

[jozefizso]

@gshpychka it's pretty valid question to understand when this feature will be released and publicly available.

@JounQin thanks for the tip. Our project is already using the eslint-plugin-import package, but I will take a look at the eslint-plugin-n project.

[gshpychka]

@gshpychka it's pretty valid question to understand when this feature will be released and publicly available.

Yes, your question was, but @JounQin's question was off-topic.

[JounQin]

I don't think it's off-topic because the two rules aim to same behavior.

[ljharb]

It’s off topic because this repo is about this project, and there’s no reason to discuss alternative projects here.