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.

Sign up for GitHub

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

Jump to bottom

feat(eslint-plugin): [no-var-requires, no-require-imports] allow option #7710

Merged
merged 5 commits into from
Jan 4, 2024
Merged

feat(eslint-plugin): [no-var-requires, no-require-imports] allow option #7710

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cc3c317
feat(eslint-plugin): [no-var-requires, no-require-imports] allowPacka…
Josh-Cena Oct 1, 2023
1fca381
Merge branch 'master' into no-var-requires-package-json
Josh-Cena Dec 18, 2023
3f67961
Use allow option
Josh-Cena Dec 18, 2023
449beae
Update snap
Josh-Cena Dec 18, 2023
b77f61d
Fix test
Josh-Cena Dec 18, 2023
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
View file
Open in desktop
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

### `allowPackageJson`
Copy link
Member

Choose a reason for hiding this comment

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

I've also seen libraries want to import from turbo.json, project.json, etc. Maybe this should be the standard format from #6017, with docs showing how to use it for package.json files? WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm okay with that. What's your proposed API? My thought is something similar to no-restricted-imports, with an array of paths.

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking the exact format from #4436 -> https://typescript-eslint.io/rules/prefer-readonly-parameter-types.

{
  "@typescript-eslint/no-var-requires": [
    "error",
    {
      ignore: [
        {
          name: "package.json",
          source: "file",
        },
      ],
    },
  ],
}

Copy link
Member Author

@Josh-Cena Josh-Cena Oct 16, 2023 •
edited

Choose a reason for hiding this comment

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

This looks couunterintuitive as a user, TBH. This API is for types, but what is "package.json" as a type? What does source: "file" even mean? With no-restricted-imports, the API would look like:

{
  "@typescript-eslint/no-var-requires": [
    "error",
    {
      "ignorePaths": [
        {
          "name": "*.json"
        }
      ]
    }
  ]
}

Copy link
Member

Choose a reason for hiding this comment

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

What happens if they want to import from a node module, e.g. import emojis from "emojilib/dist/emoji-en-US.json"? Should we just have a string[] as the option to capture all those cases? Feels off to me that we have the big thought-out format and then don't use it... but it is overhead, yes. 🤔

Copy link
Member

Choose a reason for hiding this comment

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

ping @Josh-Cena ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I did not forget this 😅 Will try to garner some energy and clear my notifications this afternoon.

Copy link
Member Author

Choose a reason for hiding this comment

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

@JoshuaKGoldberg I thought about it. Here are my thoughts:

  1. The only thing this PR changes is allowing require. It doesn't change anything about import—they can still be used as normal.
  2. Sure, we have a thought-out format for specifying types, but here we are talking about import paths, and this design doesn't come without its precedent either—we are being consistent with no-restricted-imports.
  3. About having string[] as an option. You mean { ignorePaths: string[] }? My initial design is { ignorePaths: TheOptionsOfNoRestrictedImports }, i.e. it supports multiple ignore paths, each one with a path, a glob, a custom message, etc. But I now think that's hugely unnecessary for an ignore option. So sure, I can do that.
  4. If you want to differentiate between node modules and local JSON files, you would have to support regex in the ignore path (I think) to be able to tell between ./foo/data.json and foo/data.json. That means the option has to be more granular than a string[]; it would at minimum need to be { ignorePaths: { paths: string[] /* globs */, patterns: string[] /* regexes */ }. Does that make sense? Am I overthinking it?

I'm very curious what your current idea for the option's shape is.

Copy link
Member

Choose a reason for hiding this comment

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

The intent of #6017 was: "RFC: Common format to specify a type or value as a rule option". I was really hoping it'd be usable for situations exactly like this one. But if we're seeing a practical shortcoming of the format -it being too heavy-handed- then I like this as a motivation to improve it. Or maybe it's just irrelevant here...

Maybe just allow: string[] or ignore: string[], where each of the string elements is a regex for now - and only if users ask we expand to that format? It'd be a little verbose to have to say something like "package\.json$"], but not the worst thing in the world IMO.

Thinking on the sticking point of relativity: from what I can think of either:

  • the rule gets told a root ESLint path to resolve from (but what about monorepos with config in the root vs per-package?)
  • we tell users to either increase regex specificity or allow sibling level package.json imports (/[.\\\/]*package.json/)

I think having two things to think about, like paths and patterns from 4., would be a bit much. Is there anything that can only be captured in that system, not the single option?

Copy link
Member

Choose a reason for hiding this comment

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

The complicated thing is that sometimes you have tsconfig paths which make things look non-relative. We can't build in any specific assumptions around the path here if we want people to allow local but disallow node_module packages done this way.

So a regex is probably the most flexible option for this so codebases can easily devise some regex that matches their usecase. I'd suspect that most codebases would just want something like you suggested joshg to allow a relative package.json


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}`:

<!--tabs-->

### ❌ 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.
Expand Down
22 changes: 22 additions & 0 deletions packages/eslint-plugin/docs/rules/no-var-requires.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}`:

<!--tabs-->

### ❌ 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.
Expand Down
50 changes: 45 additions & 5 deletions packages/eslint-plugin/src/rules/no-require-imports.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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<Options, MessageIds>({
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
Expand All @@ -33,6 +67,12 @@ export default util.createRule({
}
},
TSExternalModuleReference(node): void {
if (
options[0].allowPackageJson &&
isPackageJsonImport(node.expression)
) {
return;
}
context.report({
node,
messageId: 'noRequireImports',
Expand Down
29 changes: 25 additions & 4 deletions packages/eslint-plugin/src/rules/no-var-requires.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Options, MessageIds>({
Expand All @@ -17,14 +21,31 @@ export default util.createRule<Options, MessageIds>({
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
Expand Down
58 changes: 58 additions & 0 deletions packages/eslint-plugin/tests/rules/no-require-imports.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: [
{
Expand Down Expand Up @@ -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',
},
],
},
],
});
33 changes: 33 additions & 0 deletions packages/eslint-plugin/tests/rules/no-var-requires.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: [
{
Expand Down Expand Up @@ -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',
},
],
},
],
});