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(eslint-plugin): [no-var-requires, no-require-imports] allow option #7710

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 22 additions & 0 deletions packages/eslint-plugin/docs/rules/no-require-imports.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,28 @@ import { lib2 } from 'lib2';
import * as lib3 from 'lib3';
```

## Options

### `allow`

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 `{allow: ['/package\\.json$']}`:

<!--tabs-->

### ❌ Incorrect

```ts
console.log(require('../data.json').version);
```

### ✅ Correct

```ts
console.log(require('../package.json').version);
```

## When Not To Use It

If your project frequently uses older CommonJS `require`s, then this rule might not be applicable to you.
Expand Down
22 changes: 22 additions & 0 deletions packages/eslint-plugin/docs/rules/no-var-requires.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,28 @@ require('foo');
import foo from 'foo';
```

## Options

### `allow`

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 `{allow: ['/package\\.json$']}`:

<!--tabs-->

### ❌ Incorrect

```ts
const foo = require('../data.json');
```

### ✅ Correct

```ts
const foo = require('../package.json');
```

## When Not To Use It

If your project frequently uses older CommonJS `require`s, then this rule might not be applicable to you.
Expand Down
51 changes: 45 additions & 6 deletions packages/eslint-plugin/src/rules/no-require-imports.ts
Original file line number Diff line number Diff line change
@@ -1,27 +1,59 @@
import type { TSESTree } from '@typescript-eslint/utils';
import { ASTUtils } from '@typescript-eslint/utils';
import { AST_NODE_TYPES, ASTUtils } from '@typescript-eslint/utils';
import { getScope } from '@typescript-eslint/utils/eslint-utils';

import { createRule } from '../util';
import * as util from '../util';

export default createRule({
type Options = [
{
allow: string[];
},
];
type MessageIds = 'noRequireImports';

export default util.createRule<Options, MessageIds>({
name: 'no-require-imports',
meta: {
type: 'problem',
docs: {
description: 'Disallow invocation of `require()`',
},
schema: [],
schema: [
{
type: 'object',
properties: {
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: [],
create(context) {
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 (
node.arguments[0]?.type === AST_NODE_TYPES.Literal &&
typeof node.arguments[0].value === 'string' &&
isImportPathAllowed(node.arguments[0].value)
) {
Comment on lines +51 to +54
Copy link
Contributor

Choose a reason for hiding this comment

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

does it work with string constants?

const path = 'package.json';
const pkg = require(path);

Copy link
Member Author

@Josh-Cena Josh-Cena Dec 18, 2023

Choose a reason for hiding this comment

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

No, that needs to make the rule type-aware, or otherwise implement a go-to-definition mechanism, which I'm not sure we have the precedent. I'm also not sure if it happens more than a handful of times IRL.

return;
}
const variable = ASTUtils.findVariable(getScope(context), 'require');

// ignore non-global require usage as it's something user-land custom instead
Expand All @@ -34,6 +66,13 @@ export default createRule({
}
},
TSExternalModuleReference(node): void {
if (
node.expression.type === AST_NODE_TYPES.Literal &&
typeof node.expression.value === 'string' &&
isImportPathAllowed(node.expression.value)
) {
return;
}
context.report({
node,
messageId: 'noRequireImports',
Expand Down
37 changes: 33 additions & 4 deletions packages/eslint-plugin/src/rules/no-var-requires.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,11 @@ import { getScope } from '@typescript-eslint/utils/eslint-utils';

import { createRule } from '../util';

type Options = [];
type Options = [
{
allow: string[];
},
];
type MessageIds = 'noVarReqs';

export default createRule<Options, MessageIds>({
Expand All @@ -18,14 +22,39 @@ export default createRule<Options, MessageIds>({
messages: {
noVarReqs: 'Require statement not part of import statement.',
},
schema: [],
schema: [
{
type: 'object',
properties: {
allow: {
type: 'array',
items: { type: 'string' },
description: 'Patterns of import paths to allow requiring from.',
},
},
additionalProperties: false,
},
],
},
defaultOptions: [],
create(context) {
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 (
node.arguments[0]?.type === AST_NODE_TYPES.Literal &&
typeof node.arguments[0].value === 'string' &&
isImportPathAllowed(node.arguments[0].value)
Comment on lines +52 to +54
Copy link
Member

Choose a reason for hiding this comment

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

[Bug] I think this should use getStaticStringValue or an equivalent tailored just to strings. Then it would support template literals.

I did a quick search for type === AST_NODE_TYPES.Literal and found a few files Referencer.ts, prefer-regexp-exec.ts, prefer-reduce-type-parameter.ts, padding-line-between-statements.ts, no-restricted-imports.ts...) that do similar "get the value if it's a string literal" logic. Maybe good to make a new utility?

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

#8186. I wanted to get this PR merged 😄 and I don't think template literals in require() calls are pretty rare.

) {
return;
}
const parent =
node.parent.type === AST_NODE_TYPES.ChainExpression
? node.parent.parent
Expand Down
77 changes: 77 additions & 0 deletions packages/eslint-plugin/tests/rules/no-require-imports.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,30 @@ import { createRequire } from 'module';
const require = createRequire();
require('remark-preset-prettier');
`,
{
code: "const pkg = require('./package.json');",
options: [{ allow: ['/package\\.json$'] }],
},
{
code: "const pkg = require('../package.json');",
options: [{ allow: ['/package\\.json$'] }],
},
{
code: "const pkg = require('../packages/package.json');",
options: [{ allow: ['/package\\.json$'] }],
},
{
code: "import pkg = require('../packages/package.json');",
options: [{ allow: ['/package\\.json$'] }],
},
{
code: "import pkg = require('data.json');",
options: [{ allow: ['\\.json$'] }],
},
{
code: "import pkg = require('some-package');",
options: [{ allow: ['^some-package$'] }],
},
],
invalid: [
{
Expand Down Expand Up @@ -111,5 +135,58 @@ var lib5 = require?.('lib5'),
},
],
},
{
code: "const pkg = require('./package.json');",
errors: [
{
line: 1,
column: 13,
messageId: 'noRequireImports',
},
],
},
{
code: "const pkg = require('./package.jsonc');",
options: [{ allow: ['/package\\.json$'] }],
errors: [
{
line: 1,
column: 13,
messageId: 'noRequireImports',
},
],
},
{
code: "import pkg = require('./package.json');",
errors: [
{
line: 1,
column: 14,
messageId: 'noRequireImports',
},
],
},
{
code: "import pkg = require('./package.jsonc');",
options: [{ allow: ['/package\\.json$'] }],
errors: [
{
line: 1,
column: 14,
messageId: 'noRequireImports',
},
],
},
{
code: "import pkg = require('./package.json');",
options: [{ allow: ['^some-package$'] }],
errors: [
{
line: 1,
column: 14,
messageId: 'noRequireImports',
},
],
},
],
});
52 changes: 52 additions & 0 deletions packages/eslint-plugin/tests/rules/no-var-requires.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,26 @@ import { createRequire } from 'module';
const require = createRequire('foo');
const json = require('./some.json');
`,
{
code: "const pkg = require('./package.json');",
options: [{ allow: ['/package\\.json$'] }],
},
{
code: "const pkg = require('../package.json');",
options: [{ allow: ['/package\\.json$'] }],
},
{
code: "const pkg = require('../packages/package.json');",
options: [{ allow: ['/package\\.json$'] }],
},
{
code: "const pkg = require('data.json');",
options: [{ allow: ['\\.json$'] }],
},
{
code: "const pkg = require('some-package');",
options: [{ allow: ['^some-package$'] }],
},
],
invalid: [
{
Expand Down Expand Up @@ -157,5 +177,37 @@ 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: [{ allow: ['/package\\.json$'] }],
errors: [
{
line: 1,
column: 13,
messageId: 'noVarReqs',
},
],
},
{
code: "const pkg = require('./package.json');",
options: [{ allow: ['^some-package$'] }],
errors: [
{
line: 1,
column: 13,
messageId: 'noVarReqs',
},
],
},
],
});

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.