From cc3c3179b1d835093b355020c5411b404a9a1bd2 Mon Sep 17 00:00:00 2001 From: Joshua Chen Date: Sun, 1 Oct 2023 02:25:39 -0400 Subject: [PATCH 1/4] feat(eslint-plugin): [no-var-requires, no-require-imports] allowPackageJson option --- .../docs/rules/no-require-imports.md | 22 +++++++ .../docs/rules/no-var-requires.md | 22 +++++++ .../src/rules/no-require-imports.ts | 50 ++++++++++++++-- .../src/rules/no-var-requires.ts | 29 ++++++++-- .../tests/rules/no-require-imports.test.ts | 58 +++++++++++++++++++ .../tests/rules/no-var-requires.test.ts | 33 +++++++++++ 6 files changed, 205 insertions(+), 9 deletions(-) diff --git a/packages/eslint-plugin/docs/rules/no-require-imports.md b/packages/eslint-plugin/docs/rules/no-require-imports.md index 6e75a3d41cd3..b80da893e8eb 100644 --- a/packages/eslint-plugin/docs/rules/no-require-imports.md +++ b/packages/eslint-plugin/docs/rules/no-require-imports.md @@ -28,6 +28,28 @@ import { lib2 } from 'lib2'; import * as lib3 from 'lib3'; ``` +## Options + +### `allowPackageJson` + +When this is set to `true`, the rule will allow `require` imports that import `package.json` files. This is because `package.json` commonly lives outside of the TS root directory, so statically importing it would lead to root directory conflicts, especially with `resolveJsonModule` enabled. + +With `{allowPackageJson: true}`: + + + +### ❌ Incorrect + +```ts +console.log(require('../data.json').version); +``` + +### ✅ Correct + +```ts +console.log(require('../package.json').version); +``` + ## When Not To Use It If you don't care about using newer module syntax, then you will not need this rule. diff --git a/packages/eslint-plugin/docs/rules/no-var-requires.md b/packages/eslint-plugin/docs/rules/no-var-requires.md index 7230e4e8aebc..6bba20583ee5 100644 --- a/packages/eslint-plugin/docs/rules/no-var-requires.md +++ b/packages/eslint-plugin/docs/rules/no-var-requires.md @@ -28,6 +28,28 @@ require('foo'); import foo from 'foo'; ``` +## Options + +### `allowPackageJson` + +When this is set to `true`, the rule will allow `require` variables that import `package.json` files. This is because `package.json` commonly lives outside of the TS root directory, so statically importing it would lead to root directory conflicts, especially with `resolveJsonModule` enabled. + +With `{allowPackageJson: true}`: + + + +### ❌ Incorrect + +```ts +const foo = require('../data.json'); +``` + +### ✅ Correct + +```ts +const foo = require('../package.json'); +``` + ## When Not To Use It If you don't care about using newer module syntax, then you will not need this rule. diff --git a/packages/eslint-plugin/src/rules/no-require-imports.ts b/packages/eslint-plugin/src/rules/no-require-imports.ts index 2f9310b38fcd..ac02cead3510 100644 --- a/packages/eslint-plugin/src/rules/no-require-imports.ts +++ b/packages/eslint-plugin/src/rules/no-require-imports.ts @@ -1,26 +1,60 @@ import type { TSESTree } from '@typescript-eslint/utils'; -import { ASTUtils } from '@typescript-eslint/utils'; +import { AST_NODE_TYPES, ASTUtils } from '@typescript-eslint/utils'; import * as util from '../util'; -export default util.createRule({ +type Options = [ + { + allowPackageJson?: boolean; + }, +]; +type MessageIds = 'noRequireImports'; + +export default util.createRule({ name: 'no-require-imports', meta: { type: 'problem', docs: { description: 'Disallow invocation of `require()`', }, - schema: [], + schema: [ + { + type: 'object', + properties: { + allowPackageJson: { + type: 'boolean', + }, + }, + }, + ], messages: { noRequireImports: 'A `require()` style import is forbidden.', }, }, - defaultOptions: [], - create(context) { + defaultOptions: [{ allowPackageJson: false }], + create(context, options) { + function isPackageJsonImport( + specifier: TSESTree.Node | undefined, + ): boolean { + if (!specifier) { + return false; + } + return ( + specifier.type === AST_NODE_TYPES.Literal && + typeof specifier.value === 'string' && + specifier.value.endsWith('/package.json') + ); + } return { 'CallExpression[callee.name="require"]'( node: TSESTree.CallExpression, ): void { + if ( + options[0].allowPackageJson && + isPackageJsonImport(node.arguments[0]) + ) { + return; + } const variable = ASTUtils.findVariable(context.getScope(), 'require'); // ignore non-global require usage as it's something user-land custom instead @@ -33,6 +67,12 @@ export default util.createRule({ } }, TSExternalModuleReference(node): void { + if ( + options[0].allowPackageJson && + isPackageJsonImport(node.expression) + ) { + return; + } context.report({ node, messageId: 'noRequireImports', diff --git a/packages/eslint-plugin/src/rules/no-var-requires.ts b/packages/eslint-plugin/src/rules/no-var-requires.ts index b8655d049d30..0b859dc0488b 100644 --- a/packages/eslint-plugin/src/rules/no-var-requires.ts +++ b/packages/eslint-plugin/src/rules/no-var-requires.ts @@ -3,7 +3,11 @@ import { AST_NODE_TYPES, ASTUtils } from '@typescript-eslint/utils'; import * as util from '../util'; -type Options = []; +type Options = [ + { + allowPackageJson?: boolean; + }, +]; type MessageIds = 'noVarReqs'; export default util.createRule({ @@ -17,14 +21,31 @@ export default util.createRule({ messages: { noVarReqs: 'Require statement not part of import statement.', }, - schema: [], + schema: [ + { + type: 'object', + properties: { + allowPackageJson: { + type: 'boolean', + }, + }, + }, + ], }, - defaultOptions: [], - create(context) { + defaultOptions: [{ allowPackageJson: false }], + create(context, options) { return { 'CallExpression[callee.name="require"]'( node: TSESTree.CallExpression, ): void { + if ( + options[0].allowPackageJson && + node.arguments[0]?.type === AST_NODE_TYPES.Literal && + typeof node.arguments[0].value === 'string' && + node.arguments[0].value.endsWith('/package.json') + ) { + return; + } const parent = node.parent?.type === AST_NODE_TYPES.ChainExpression ? node.parent.parent diff --git a/packages/eslint-plugin/tests/rules/no-require-imports.test.ts b/packages/eslint-plugin/tests/rules/no-require-imports.test.ts index ff6cbd2a6032..ceb11f4e080c 100644 --- a/packages/eslint-plugin/tests/rules/no-require-imports.test.ts +++ b/packages/eslint-plugin/tests/rules/no-require-imports.test.ts @@ -23,6 +23,22 @@ import { createRequire } from 'module'; const require = createRequire(); require('remark-preset-prettier'); `, + { + code: "const pkg = require('./package.json');", + options: [{ allowPackageJson: true }], + }, + { + code: "const pkg = require('../package.json');", + options: [{ allowPackageJson: true }], + }, + { + code: "const pkg = require('../packages/package.json');", + options: [{ allowPackageJson: true }], + }, + { + code: "import pkg = require('../packages/package.json');", + options: [{ allowPackageJson: true }], + }, ], invalid: [ { @@ -111,5 +127,47 @@ var lib5 = require?.('lib5'), }, ], }, + { + code: "const pkg = require('./package.json');", + errors: [ + { + line: 1, + column: 13, + messageId: 'noRequireImports', + }, + ], + }, + { + code: "const pkg = require('./package.jsonc');", + options: [{ allowPackageJson: true }], + errors: [ + { + line: 1, + column: 13, + messageId: 'noRequireImports', + }, + ], + }, + { + code: "import pkg = require('./package.json');", + errors: [ + { + line: 1, + column: 13, + messageId: 'noRequireImports', + }, + ], + }, + { + code: "import pkg = require('./package.jsonc');", + options: [{ allowPackageJson: true }], + errors: [ + { + line: 1, + column: 14, + messageId: 'noRequireImports', + }, + ], + }, ], }); diff --git a/packages/eslint-plugin/tests/rules/no-var-requires.test.ts b/packages/eslint-plugin/tests/rules/no-var-requires.test.ts index 14ce51109941..71c09287d0f8 100644 --- a/packages/eslint-plugin/tests/rules/no-var-requires.test.ts +++ b/packages/eslint-plugin/tests/rules/no-var-requires.test.ts @@ -16,6 +16,18 @@ import { createRequire } from 'module'; const require = createRequire('foo'); const json = require('./some.json'); `, + { + code: "const pkg = require('./package.json');", + options: [{ allowPackageJson: true }], + }, + { + code: "const pkg = require('../package.json');", + options: [{ allowPackageJson: true }], + }, + { + code: "const pkg = require('../packages/package.json');", + options: [{ allowPackageJson: true }], + }, ], invalid: [ { @@ -157,5 +169,26 @@ configValidator.addSchema(require('./a.json')); }, ], }, + { + code: "const pkg = require('./package.json');", + errors: [ + { + line: 1, + column: 13, + messageId: 'noVarReqs', + }, + ], + }, + { + code: "const pkg = require('./package.jsonc');", + options: [{ allowPackageJson: true }], + errors: [ + { + line: 1, + column: 13, + messageId: 'noVarReqs', + }, + ], + }, ], }); From 3f67961176e7f5d5c9e8f43c10c7f3a2d9c18fee Mon Sep 17 00:00:00 2001 From: Joshua Chen Date: Mon, 18 Dec 2023 13:07:11 -0500 Subject: [PATCH 2/4] Use allow option --- .../docs/rules/no-require-imports.md | 6 +-- .../docs/rules/no-var-requires.md | 6 +-- .../src/rules/no-require-imports.ts | 37 +++++++++---------- .../src/rules/no-var-requires.ts | 20 +++++++--- .../tests/rules/no-require-imports.test.ts | 31 +++++++++++++--- .../tests/rules/no-var-requires.test.ts | 27 ++++++++++++-- 6 files changed, 86 insertions(+), 41 deletions(-) diff --git a/packages/eslint-plugin/docs/rules/no-require-imports.md b/packages/eslint-plugin/docs/rules/no-require-imports.md index 7a2b9b6f8344..d67d639b9cb8 100644 --- a/packages/eslint-plugin/docs/rules/no-require-imports.md +++ b/packages/eslint-plugin/docs/rules/no-require-imports.md @@ -30,11 +30,11 @@ import * as lib3 from 'lib3'; ## Options -### `allowPackageJson` +### `allow` -When this is set to `true`, the rule will allow `require` imports that import `package.json` files. This is because `package.json` commonly lives outside of the TS root directory, so statically importing it would lead to root directory conflicts, especially with `resolveJsonModule` enabled. +A array of strings. These strings will be compiled into regular expressions with the `u` flag and be used to test against the imported path. A common use case is to allow importing `package.json`. This is because `package.json` commonly lives outside of the TS root directory, so statically importing it would lead to root directory conflicts, especially with `resolveJsonModule` enabled. You can also use it to allow importing any JSON if your environment doesn't support JSON modules, or use it for other cases where `import` statements cannot work. -With `{allowPackageJson: true}`: +With `{allow: ['/package\\.json$']}`: diff --git a/packages/eslint-plugin/docs/rules/no-var-requires.md b/packages/eslint-plugin/docs/rules/no-var-requires.md index 8a7628d460b6..0bd0b2b564b9 100644 --- a/packages/eslint-plugin/docs/rules/no-var-requires.md +++ b/packages/eslint-plugin/docs/rules/no-var-requires.md @@ -30,11 +30,11 @@ import foo from 'foo'; ## Options -### `allowPackageJson` +### `allow` -When this is set to `true`, the rule will allow `require` variables that import `package.json` files. This is because `package.json` commonly lives outside of the TS root directory, so statically importing it would lead to root directory conflicts, especially with `resolveJsonModule` enabled. +A array of strings. These strings will be compiled into regular expressions with the `u` flag and be used to test against the imported path. A common use case is to allow importing `package.json`. This is because `package.json` commonly lives outside of the TS root directory, so statically importing it would lead to root directory conflicts, especially with `resolveJsonModule` enabled. You can also use it to allow importing any JSON if your environment doesn't support JSON modules, or use it for other cases where `import` statements cannot work. -With `{allowPackageJson: true}`: +With `{allow: ['/package\\.json$']}`: diff --git a/packages/eslint-plugin/src/rules/no-require-imports.ts b/packages/eslint-plugin/src/rules/no-require-imports.ts index a5c214c08150..c4f20e6c68f7 100644 --- a/packages/eslint-plugin/src/rules/no-require-imports.ts +++ b/packages/eslint-plugin/src/rules/no-require-imports.ts @@ -6,7 +6,7 @@ import * as util from '../util'; type Options = [ { - allowPackageJson?: boolean; + allow: string[]; }, ]; type MessageIds = 'noRequireImports'; @@ -22,37 +22,35 @@ export default util.createRule({ { type: 'object', properties: { - allowPackageJson: { - type: 'boolean', + allow: { + type: 'array', + items: { type: 'string' }, + description: 'Patterns of import paths to allow requiring from.', }, }, + additionalProperties: false, }, ], messages: { noRequireImports: 'A `require()` style import is forbidden.', }, }, - defaultOptions: [{ allowPackageJson: false }], + defaultOptions: [{ allow: [] }], create(context, options) { - function isPackageJsonImport( - specifier: TSESTree.Node | undefined, - ): boolean { - if (!specifier) { - return false; - } - return ( - specifier.type === AST_NODE_TYPES.Literal && - typeof specifier.value === 'string' && - specifier.value.endsWith('/package.json') - ); + const allowPatterns = options[0].allow.map( + pattern => new RegExp(pattern, 'u'), + ); + function isImportPathAllowed(importPath: string): boolean { + return allowPatterns.some(pattern => importPath.match(pattern)); } return { 'CallExpression[callee.name="require"]'( node: TSESTree.CallExpression, ): void { if ( - options[0].allowPackageJson && - isPackageJsonImport(node.arguments[0]) + node.arguments[0]?.type === AST_NODE_TYPES.Literal && + typeof node.arguments[0].value === 'string' && + isImportPathAllowed(node.arguments[0].value) ) { return; } @@ -69,8 +67,9 @@ export default util.createRule({ }, TSExternalModuleReference(node): void { if ( - options[0].allowPackageJson && - isPackageJsonImport(node.expression) + node.expression.type === AST_NODE_TYPES.Literal && + typeof node.expression.value === 'string' && + isImportPathAllowed(node.expression.value) ) { return; } diff --git a/packages/eslint-plugin/src/rules/no-var-requires.ts b/packages/eslint-plugin/src/rules/no-var-requires.ts index 28913c6fb79d..3605904e655b 100644 --- a/packages/eslint-plugin/src/rules/no-var-requires.ts +++ b/packages/eslint-plugin/src/rules/no-var-requires.ts @@ -6,7 +6,7 @@ import { createRule } from '../util'; type Options = [ { - allowPackageJson?: boolean; + allow: string[]; }, ]; type MessageIds = 'noVarReqs'; @@ -26,24 +26,32 @@ export default createRule({ { type: 'object', properties: { - allowPackageJson: { - type: 'boolean', + allow: { + type: 'array', + items: { type: 'string' }, + description: 'Patterns of import paths to allow requiring from.', }, }, + additionalProperties: false, }, ], }, - defaultOptions: [{ allowPackageJson: false }], + defaultOptions: [{ allow: [] }], create(context, options) { + const allowPatterns = options[0].allow.map( + pattern => new RegExp(pattern, 'u'), + ); + function isImportPathAllowed(importPath: string): boolean { + return allowPatterns.some(pattern => importPath.match(pattern)); + } return { 'CallExpression[callee.name="require"]'( node: TSESTree.CallExpression, ): void { if ( - options[0].allowPackageJson && node.arguments[0]?.type === AST_NODE_TYPES.Literal && typeof node.arguments[0].value === 'string' && - node.arguments[0].value.endsWith('/package.json') + isImportPathAllowed(node.arguments[0].value) ) { return; } diff --git a/packages/eslint-plugin/tests/rules/no-require-imports.test.ts b/packages/eslint-plugin/tests/rules/no-require-imports.test.ts index ceb11f4e080c..360478fcdf89 100644 --- a/packages/eslint-plugin/tests/rules/no-require-imports.test.ts +++ b/packages/eslint-plugin/tests/rules/no-require-imports.test.ts @@ -25,19 +25,27 @@ require('remark-preset-prettier'); `, { code: "const pkg = require('./package.json');", - options: [{ allowPackageJson: true }], + options: [{ allow: ['/package\\.json$'] }], }, { code: "const pkg = require('../package.json');", - options: [{ allowPackageJson: true }], + options: [{ allow: ['/package\\.json$'] }], }, { code: "const pkg = require('../packages/package.json');", - options: [{ allowPackageJson: true }], + options: [{ allow: ['/package\\.json$'] }], }, { code: "import pkg = require('../packages/package.json');", - options: [{ allowPackageJson: true }], + options: [{ allow: ['/package\\.json$'] }], + }, + { + code: "import pkg = require('data.json');", + options: [{ allow: ['\\.json$'] }], + }, + { + code: "import pkg = require('some-package');", + options: [{ allow: ['^some-package$'] }], }, ], invalid: [ @@ -139,7 +147,7 @@ var lib5 = require?.('lib5'), }, { code: "const pkg = require('./package.jsonc');", - options: [{ allowPackageJson: true }], + options: [{ allow: ['/package\\.json$'] }], errors: [ { line: 1, @@ -160,7 +168,7 @@ var lib5 = require?.('lib5'), }, { code: "import pkg = require('./package.jsonc');", - options: [{ allowPackageJson: true }], + options: [{ allow: ['/package\\.json$'] }], errors: [ { line: 1, @@ -169,5 +177,16 @@ var lib5 = require?.('lib5'), }, ], }, + { + code: "import pkg = require('./package.json');", + options: [{ allow: ['^some-package$'] }], + errors: [ + { + line: 1, + column: 13, + messageId: 'noRequireImports', + }, + ], + }, ], }); diff --git a/packages/eslint-plugin/tests/rules/no-var-requires.test.ts b/packages/eslint-plugin/tests/rules/no-var-requires.test.ts index 71c09287d0f8..718bcc46db31 100644 --- a/packages/eslint-plugin/tests/rules/no-var-requires.test.ts +++ b/packages/eslint-plugin/tests/rules/no-var-requires.test.ts @@ -18,15 +18,23 @@ const json = require('./some.json'); `, { code: "const pkg = require('./package.json');", - options: [{ allowPackageJson: true }], + options: [{ allow: ['/package\\.json$'] }], }, { code: "const pkg = require('../package.json');", - options: [{ allowPackageJson: true }], + options: [{ allow: ['/package\\.json$'] }], }, { code: "const pkg = require('../packages/package.json');", - options: [{ allowPackageJson: true }], + options: [{ allow: ['/package\\.json$'] }], + }, + { + code: "const pkg = require('data.json');", + options: [{ allow: ['\\.json$'] }], + }, + { + code: "const pkg = require('some-package');", + options: [{ allow: ['^some-package$'] }], }, ], invalid: [ @@ -181,7 +189,18 @@ configValidator.addSchema(require('./a.json')); }, { code: "const pkg = require('./package.jsonc');", - options: [{ allowPackageJson: true }], + options: [{ allow: ['/package\\.json$'] }], + errors: [ + { + line: 1, + column: 13, + messageId: 'noVarReqs', + }, + ], + }, + { + code: "const pkg = require('./package.json');", + options: [{ allow: ['^some-package$'] }], errors: [ { line: 1, From 449beaea13feadeb41e461a60fa5e8ca93402eb4 Mon Sep 17 00:00:00 2001 From: Joshua Chen Date: Mon, 18 Dec 2023 13:38:20 -0500 Subject: [PATCH 3/4] Update snap --- .../schema-snapshots/no-require-imports.shot | 25 ++++++++++++++++--- .../schema-snapshots/no-var-requires.shot | 25 ++++++++++++++++--- 2 files changed, 44 insertions(+), 6 deletions(-) diff --git a/packages/eslint-plugin/tests/schema-snapshots/no-require-imports.shot b/packages/eslint-plugin/tests/schema-snapshots/no-require-imports.shot index bdb7cb325381..7561bcbc4b3c 100644 --- a/packages/eslint-plugin/tests/schema-snapshots/no-require-imports.shot +++ b/packages/eslint-plugin/tests/schema-snapshots/no-require-imports.shot @@ -4,11 +4,30 @@ exports[`Rule schemas should be convertible to TS types for documentation purpos " # SCHEMA: -[] +[ + { + "additionalProperties": false, + "properties": { + "allow": { + "description": "Patterns of import paths to allow requiring from.", + "items": { + "type": "string" + }, + "type": "array" + } + }, + "type": "object" + } +] # TYPES: -/** No options declared */ -type Options = [];" +type Options = [ + { + /** Patterns of import paths to allow requiring from. */ + allow?: string[]; + }, +]; +" `; diff --git a/packages/eslint-plugin/tests/schema-snapshots/no-var-requires.shot b/packages/eslint-plugin/tests/schema-snapshots/no-var-requires.shot index 992833a20ae3..5271679f9bbb 100644 --- a/packages/eslint-plugin/tests/schema-snapshots/no-var-requires.shot +++ b/packages/eslint-plugin/tests/schema-snapshots/no-var-requires.shot @@ -4,11 +4,30 @@ exports[`Rule schemas should be convertible to TS types for documentation purpos " # SCHEMA: -[] +[ + { + "additionalProperties": false, + "properties": { + "allow": { + "description": "Patterns of import paths to allow requiring from.", + "items": { + "type": "string" + }, + "type": "array" + } + }, + "type": "object" + } +] # TYPES: -/** No options declared */ -type Options = [];" +type Options = [ + { + /** Patterns of import paths to allow requiring from. */ + allow?: string[]; + }, +]; +" `; From b77f61ddc01e165f6bcb7408fd5f64fe5cb84ed5 Mon Sep 17 00:00:00 2001 From: Joshua Chen Date: Mon, 18 Dec 2023 14:46:31 -0500 Subject: [PATCH 4/4] Fix test --- packages/eslint-plugin/tests/rules/no-require-imports.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/eslint-plugin/tests/rules/no-require-imports.test.ts b/packages/eslint-plugin/tests/rules/no-require-imports.test.ts index 360478fcdf89..42374ccffbf8 100644 --- a/packages/eslint-plugin/tests/rules/no-require-imports.test.ts +++ b/packages/eslint-plugin/tests/rules/no-require-imports.test.ts @@ -161,7 +161,7 @@ var lib5 = require?.('lib5'), errors: [ { line: 1, - column: 13, + column: 14, messageId: 'noRequireImports', }, ], @@ -183,7 +183,7 @@ var lib5 = require?.('lib5'), errors: [ { line: 1, - column: 13, + column: 14, messageId: 'noRequireImports', }, ],