[New] add enforce-node-protocol-usage rule

[GoldStrikeArch]

Fixes #2717
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 #3024 (comment), 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.

[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]

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 ⚠️

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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.