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

fix(eslint-plugin): [no-var-requires, no-require-imports] support template literal #8408

Merged
merged 8 commits into from Mar 6, 2024
Merged

fix(eslint-plugin): [no-var-requires, no-require-imports] support template literal #8408

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d8d0aad
fix(eslint-plugin): [no-var-requires, no-require-imports] support tem…
yeonjuan Feb 8, 2024
e38f845
refactor
yeonjuan Feb 8, 2024
c87c00f
Merge branch 'main' into fix/8186
yeonjuan Feb 12, 2024
fb16063
Merge branch 'main' into fix/8186
yeonjuan Feb 13, 2024
89d0124
Merge branch 'main' into fix/8186
yeonjuan Feb 20, 2024
c7f0048
refactor
yeonjuan Feb 20, 2024
a6ef798
Merge branch 'main' into fix/8186
yeonjuan Mar 4, 2024
392b61d
Merge branch 'main' into fix/8186
yeonjuan Mar 6, 2024
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
30 changes: 18 additions & 12 deletions packages/eslint-plugin/src/rules/no-require-imports.ts
View file
Open in desktop
Expand Up @@ -42,16 +42,23 @@ export default util.createRule<Options, MessageIds>({
function isImportPathAllowed(importPath: string): boolean {
return allowPatterns.some(pattern => importPath.match(pattern));
}
function isStringOrTemplateLiteral(node: TSESTree.Node): boolean {
return (
(node.type === AST_NODE_TYPES.Literal &&
typeof node.value === 'string') ||
node.type === AST_NODE_TYPES.TemplateLiteral
);
}

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)
) {
return;
if (node.arguments[0] && isStringOrTemplateLiteral(node.arguments[0])) {
const argValue = util.getStaticStringValue(node.arguments[0]);
if (typeof argValue === 'string' && isImportPathAllowed(argValue)) {
return;
}
}
const variable = ASTUtils.findVariable(
context.sourceCode.getScope(node),
Expand All @@ -68,12 +75,11 @@ export default util.createRule<Options, MessageIds>({
}
},
TSExternalModuleReference(node): void {
if (
node.expression.type === AST_NODE_TYPES.Literal &&
typeof node.expression.value === 'string' &&
isImportPathAllowed(node.expression.value)
) {
return;
if (isStringOrTemplateLiteral(node.expression)) {
const argValue = util.getStaticStringValue(node.expression);
if (typeof argValue === 'string' && isImportPathAllowed(argValue)) {
return;
}
}
context.report({
node,
Expand Down
22 changes: 15 additions & 7 deletions packages/eslint-plugin/src/rules/no-var-requires.ts
View file
Open in desktop
@@ -1,7 +1,7 @@
import type { TSESTree } from '@typescript-eslint/utils';
import { AST_NODE_TYPES, ASTUtils } from '@typescript-eslint/utils';

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

type Options = [
{
Expand Down Expand Up @@ -43,16 +43,24 @@ export default createRule<Options, MessageIds>({
function isImportPathAllowed(importPath: string): boolean {
return allowPatterns.some(pattern => importPath.match(pattern));
}

function isStringOrTemplateLiteral(node: TSESTree.Node): boolean {
return (
(node.type === AST_NODE_TYPES.Literal &&
typeof node.value === 'string') ||
node.type === AST_NODE_TYPES.TemplateLiteral
);
}

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)
) {
return;
if (node.arguments[0] && isStringOrTemplateLiteral(node.arguments[0])) {
const argValue = getStaticStringValue(node.arguments[0]);
Comment on lines +59 to +60
Copy link
Member

Choose a reason for hiding this comment

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

[non-actionable] getStaticStringValue internally checks for node.type === AST_NODE_TYPES.Literal || node.type === AST_NODE_TYPES.TemplateLiteral, so calling isStringOrTemplateLiteral seems a bit redundant here.. Although it returns modified values for non-string literals, but in this particular case (with the require call) I think it is fine, since passing non-string values to the require call would result in a TS error.

I'm fine with the current implementation, just leaving my thoughts here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @auvred Thanks for sharing your thoughts.
you're right, I added that condition because I was concerned about cases where non-strings(regex, number,..) are used in the arguments of the require() (probably no one writes code like that).
I thought it would be nice if TypeScript Eslint didn't throw an error when non-string value is used and a TS error occurs.

Copy link
Member

Choose a reason for hiding this comment

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

didn't throw an error when (invalid thing)

Aha, yes, that's a good thing to look out for in general. This is a very good + timely thread! 😄 Thanks - #8549

if (typeof argValue === 'string' && isImportPathAllowed(argValue)) {
return;
}
}
const parent =
node.parent.type === AST_NODE_TYPES.ChainExpression
Expand Down
30 changes: 30 additions & 0 deletions packages/eslint-plugin/tests/rules/no-require-imports.test.ts
View file
Open in desktop
Expand Up @@ -31,6 +31,10 @@ 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$'] }],
Expand All @@ -47,6 +51,10 @@ require('remark-preset-prettier');
code: "import pkg = require('some-package');",
options: [{ allow: ['^some-package$'] }],
},
{
code: 'import pkg = require(`some-package`);',
options: [{ allow: ['^some-package$'] }],
},
],
invalid: [
{
Expand Down Expand Up @@ -156,6 +164,17 @@ var lib5 = require?.('lib5'),
},
],
},
{
code: 'const pkg = require(`./package.jsonc`);',
options: [{ allow: ['/package\\.json$'] }],
errors: [
{
line: 1,
column: 13,
messageId: 'noRequireImports',
},
],
},
{
code: "import pkg = require('./package.json');",
errors: [
Expand Down Expand Up @@ -188,5 +207,16 @@ var lib5 = require?.('lib5'),
},
],
},
{
code: 'import pkg = require(`./package.json`);',
options: [{ allow: ['^some-package$'] }],
errors: [
{
line: 1,
column: 14,
messageId: 'noRequireImports',
},
],
},
],
});
15 changes: 15 additions & 0 deletions packages/eslint-plugin/tests/rules/no-var-requires.test.ts
View file
Open in desktop
Expand Up @@ -36,6 +36,10 @@ const json = require('./some.json');
code: "const pkg = require('some-package');",
options: [{ allow: ['^some-package$'] }],
},
{
code: 'const pkg = require(`some-package`);',
options: [{ allow: ['^some-package$'] }],
},
],
invalid: [
{
Expand Down Expand Up @@ -209,5 +213,16 @@ configValidator.addSchema(require('./a.json'));
},
],
},
{
code: 'const pkg = require(`./package.json`);',
options: [{ allow: ['^some-package$'] }],
errors: [
{
line: 1,
column: 13,
messageId: 'noVarReqs',
},
],
},
],
});