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

feat: added prefer-node-builtin-imports rule #3024

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions config/recommended.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ module.exports = {
'import/no-named-as-default': 'warn',
'import/no-named-as-default-member': 'warn',
'import/no-duplicates': 'warn',
'import/prefer-node-builtin-imports': 'warn',
Copy link
Member

Choose a reason for hiding this comment

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

adding things to recommended is a breaking change

},

// need all these for parsing dependencies (even if _your_ code doesn't need
Expand Down
39 changes: 39 additions & 0 deletions docs/rules/prefer-node-builtin-imports.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
# import/prefer-node-builtin-imports
Copy link
Member

Choose a reason for hiding this comment

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

I’m not interested in a rule that solely pushes using the protocol - if anything, it’d be a rule that can be configured to require, or forbid, the protocol.

Copy link
Author

@GoldStrikeArch GoldStrikeArch Jul 4, 2024

Choose a reason for hiding this comment

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

@ljharb What config option should be from user's perspective? Just a boolean to toggle this? If yes then I don't see how is it different from simply have it on or off

Copy link
Member

Choose a reason for hiding this comment

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

"off" is the current state - where you can have import 'fs' and import 'node:path' in the same file. One rule setting would error on the first one, and require node:fs. The other would error on the second one, and require path.

Copy link
Author

Choose a reason for hiding this comment

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

@ljharb Ok, got it.


🔧 This rule is automatically fixable by the [`--fix` CLI option](https://eslint.org/docs/latest/user-guide/command-line-interface#--fix).

<!-- end auto-generated rule header -->

Reports when there is no `node:` protocol for builtin modules.

```ts
import path from "node:path";
```

## Rule Details

This rule enforces that builtins node imports are using `node:` protocol. It resolved the conflict of a module (npm-installed) in `node_modules` overriding the built-in module. Besides that, it is also clear that a built-in Node.js module is imported.

## Examples

❌ Invalid

```ts
import fs from "fs";
export { promises } from "fs";
// require
const fs = require("fs/promises");
```

✅ Valid

```ts
import fs from "node:fs";
export { promises } from "node:fs";
// require
const fs = require("node:fs/promises");
```

## When Not To Use It

If you are using browser or Bun or Deno since this rule doesn't do anything with them.
1 change: 1 addition & 0 deletions src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ export const rules = {
'dynamic-import-chunkname': require('./rules/dynamic-import-chunkname'),
'no-import-module-exports': require('./rules/no-import-module-exports'),
'no-empty-named-blocks': require('./rules/no-empty-named-blocks'),
'prefer-node-builtin-imports': require('./rules/prefer-node-builtin-imports'),

// export
'exports-last': require('./rules/exports-last'),
Expand Down
101 changes: 101 additions & 0 deletions src/rules/prefer-node-builtin-imports.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
'use strict';

const { builtinModules } = require('module');
const { default: docsUrl } = require('../docsUrl');

const MESSAGE_ID = 'prefer-node-builtin-imports';
const messages = {
[MESSAGE_ID]: 'Prefer `node:{{moduleName}}` over `{{moduleName}}`.',
};

function replaceStringLiteral(
fixer,
node,
text,
relativeRangeStart,
relativeRangeEnd,
) {
const firstCharacterIndex = node.range[0] + 1;
const start = Number.isInteger(relativeRangeEnd)
? relativeRangeStart + firstCharacterIndex
: firstCharacterIndex;
const end = Number.isInteger(relativeRangeEnd)
? relativeRangeEnd + firstCharacterIndex
: node.range[1] - 1;

return fixer.replaceTextRange([start, end], text);
}

const isStringLiteral = (node) => node.type === 'Literal' && typeof node.value === 'string';

const isStaticRequireWith1Param = (node) => !node.optional
&& node.callee.type === 'Identifier'
&& node.callee.name === 'require'
&& node.arguments[0]
// check for only 1 argument
&& !node.arguments[1];

function checkAndReport(src, ctx) {
const { value } = src;

if (!builtinModules.includes(value)) { return; }

if (value.startsWith('node:')) { return; }

ctx.report({
node: src,
messageId: MESSAGE_ID,
data: { moduleName: value },
/** @param {import('eslint').Rule.RuleFixer} fixer */
fix(fixer) {
return replaceStringLiteral(fixer, src, 'node:', 0, 0);
},
});
}

/** @type {import('eslint').Rule.RuleModule} */
module.exports = {
meta: {
type: 'suggestion',
docs: {
description:
'Prefer using the `node:` protocol when importing Node.js builtin modules.',
recommended: true,
category: 'Best Practices',
url: docsUrl('prefer-node-builin-imports'),
},
fixable: 'code',
schema: [],
messages,
},
create(ctx) {
return {
CallExpression(node) {
if (!isStaticRequireWith1Param(node)) {
return;
}

if (!isStringLiteral(node.arguments[0])) {
return;
}

return checkAndReport(node.arguments[0], ctx);
},
ExportNamedDeclaration(node) {
if (!isStringLiteral) { return; }

return checkAndReport(node.source, ctx);
},
ImportDeclaration(node) {
if (!isStringLiteral) { return; }

return checkAndReport(node.source, ctx);
},
ImportExpression(node) {
if (!isStringLiteral) { return; }

return checkAndReport(node.source, ctx);
},
};
},
};
83 changes: 83 additions & 0 deletions tests/src/rules/prefer-node-builtin-imports.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
import { test } from '../utils';

import { RuleTester } from 'eslint';

const ruleTester = new RuleTester();
const rule = require('rules/prefer-node-builtin-imports');

ruleTester.run('prefer-node-builtin-imports', rule, {
valid: [
test({ code: 'import unicorn from "unicorn";' }),
test({ code: 'import fs from "./fs";' }),
test({ code: 'import fs from "unknown-builtin-module";' }),
test({ code: 'import fs from "node:fs";' }),
test({
code: `
async function foo() {
const fs = await import(fs);
}`,
}),
test({
code: `
async function foo() {
const fs = await import(0);
}`,
}),
test({
code: `
async function foo() {
const fs = await import(\`fs\`);
}`,
}),
test({ code: 'import "punycode/";' }),
test({ code: 'const fs = require("node:fs");' }),
test({ code: 'const fs = require("node:fs/promises");' }),
test({ code: 'const fs = require(fs);' }),
test({ code: 'const fs = notRequire("fs");' }),
test({ code: 'const fs = foo.require("fs");' }),
test({ code: 'const fs = require.resolve("fs");' }),
test({ code: 'const fs = require(`fs`);' }),
test({ code: 'const fs = require?.("fs");' }),
test({ code: 'const fs = require("fs", extra);' }),
test({ code: 'const fs = require();' }),
test({ code: 'const fs = require(...["fs"]);' }),
test({ code: 'const fs = require("unicorn");' }),
],
invalid: [
test({ code: 'import fs from "fs";' }),
test({ code: 'export {promises} from "fs";' }),
test({
code: `
async function foo() {
const fs = await import('fs');
}`,
}),
test({ code: 'import fs from "fs/promises";' }),
test({ code: 'export {default} from "fs/promises";' }),
test({
code: `
async function foo() {
const fs = await import('fs/promises');
}`,
}),
test({ code: 'import {promises} from "fs";' }),
test({ code: 'export {default as promises} from "fs";' }),
test({
code: `
async function foo() {
const fs = await import("fs/promises");
}`,
}),
test({
code: `
async function foo() {
const fs = await import(/* escaped */"\\u{66}s/promises");
`,
}),
test({ code: 'import "buffer";' }),
test({ code: 'import "child_process";' }),
test({ code: 'import "timers/promises";' }),
test({ code: 'const {promises} = require("fs")' }),
test({ code: 'const fs = require("fs/promises")' }),
],
});
Loading