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

[Generic Import Callback] Make callback for all import once in rules #1237

Merged
merged 1 commit into from Jan 23, 2021
Merged

[Generic Import Callback] Make callback for all import once in rules #1237

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 5 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Expand Up @@ -15,6 +15,9 @@ This change log adheres to standards from [Keep a CHANGELOG](http://keepachangel
- [`no-unused-modules`]: make type imports mark a module as used (fixes #1924) ([#1974], thanks [@cherryblossom000])
- [`import/no-cycle`]: fix perf regression ([#1944], thanks [@Blasz])

### Changed
- [Generic Import Callback] Make callback for all imports once in rules ([#1237], thanks [@ljqx])

## [2.22.1] - 2020-09-27
### Fixed
- [`default`]/TypeScript: avoid crash on `export =` with a MemberExpression ([#1841], thanks [@ljharb])
Expand Down Expand Up @@ -867,6 +870,7 @@ for info on changes for earlier releases.
[#1253]: https://github.com/benmosher/eslint-plugin-import/pull/1253
[#1248]: https://github.com/benmosher/eslint-plugin-import/pull/1248
[#1238]: https://github.com/benmosher/eslint-plugin-import/pull/1238
[#1237]: https://github.com/benmosher/eslint-plugin-import/pull/1237
[#1235]: https://github.com/benmosher/eslint-plugin-import/pull/1235
[#1234]: https://github.com/benmosher/eslint-plugin-import/pull/1234
[#1232]: https://github.com/benmosher/eslint-plugin-import/pull/1232
Expand Down Expand Up @@ -1295,4 +1299,4 @@ for info on changes for earlier releases.
[@straub]: https://github.com/straub
[@andreubotella]: https://github.com/andreubotella
[@cherryblossom000]: https://github.com/cherryblossom000
[@Blasz]: https://github.com/Blasz
[@Blasz]: https://github.com/Blasz
12 changes: 4 additions & 8 deletions src/rules/extensions.js
View file
Open in desktop
Expand Up @@ -2,6 +2,7 @@ import path from 'path';

import resolve from 'eslint-module-utils/resolve';
import { isBuiltIn, isExternalModule, isScoped, isScopedModule } from '../core/importType';
import moduleVisitor from 'eslint-module-utils/moduleVisitor';
import docsUrl from '../docsUrl';

const enumValues = { enum: [ 'always', 'ignorePackages', 'never' ] };
Expand Down Expand Up @@ -134,12 +135,10 @@ module.exports = {
return false;
}

function checkFileExtension(node) {
const { source } = node;

function checkFileExtension(source) {
// bail if the declaration doesn't have a source, e.g. "export { foo };"
if (!source) return;

const importPathWithQueryString = source.value;

// don't enforce anything on builtins
Expand Down Expand Up @@ -181,9 +180,6 @@ module.exports = {
}
}

return {
ImportDeclaration: checkFileExtension,
ExportNamedDeclaration: checkFileExtension,
};
return moduleVisitor(checkFileExtension, { commonjs: true });
},
};
22 changes: 6 additions & 16 deletions src/rules/max-dependencies.js
View file
Open in desktop
@@ -1,4 +1,4 @@
import isStaticRequire from '../core/staticRequire';
import moduleVisitor from 'eslint-module-utils/moduleVisitor';
import docsUrl from '../docsUrl';

const DEFAULT_MAX = 10;
Expand Down Expand Up @@ -36,23 +36,13 @@ module.exports = {
const dependencies = new Set(); // keep track of dependencies
let lastNode; // keep track of the last node to report on

return {
ImportDeclaration(node) {
dependencies.add(node.source.value);
lastNode = node.source;
},

CallExpression(node) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

here this PR changes the behavior, but according to the code, it should be a hidden bug and this change fixes it

if (isStaticRequire(node)) {
const [ requirePath ] = node.arguments;
dependencies.add(requirePath.value);
lastNode = node;
}
},

return Object.assign({
'Program:exit': function () {
countDependencies(dependencies, lastNode, context);
},
};
}, moduleVisitor((source) => {
dependencies.add(source.value);
lastNode = source;
}, { commonjs: true }));
},
};
4 changes: 2 additions & 2 deletions src/rules/no-extraneous-dependencies.js
View file
Open in desktop
Expand Up @@ -207,8 +207,8 @@ module.exports = {
allowBundledDeps: testConfig(options.bundledDependencies, filename) !== false,
};

return moduleVisitor(node => {
reportIfMissing(context, deps, depsOptions, node, node.value);
return moduleVisitor((source, node) => {
reportIfMissing(context, deps, depsOptions, node, source.value);
}, { commonjs: true });
},
};
24 changes: 4 additions & 20 deletions src/rules/no-internal-modules.js
View file
Open in desktop
Expand Up @@ -2,7 +2,7 @@ import minimatch from 'minimatch';

import resolve from 'eslint-module-utils/resolve';
import importType from '../core/importType';
import isStaticRequire from '../core/staticRequire';
import moduleVisitor from 'eslint-module-utils/moduleVisitor';
import docsUrl from '../docsUrl';

module.exports = {
Expand Down Expand Up @@ -87,24 +87,8 @@ module.exports = {
}
}

return {
ImportDeclaration(node) {
checkImportForReaching(node.source.value, node.source);
},
ExportAllDeclaration(node) {
checkImportForReaching(node.source.value, node.source);
},
ExportNamedDeclaration(node) {
if (node.source) {
checkImportForReaching(node.source.value, node.source);
}
},
CallExpression(node) {
if (isStaticRequire(node)) {
const [ firstArgument ] = node.arguments;
checkImportForReaching(firstArgument.value, firstArgument);
}
},
};
return moduleVisitor((source) => {
checkImportForReaching(source.value, source);
}, { commonjs: true });
},
};
15 changes: 4 additions & 11 deletions src/rules/no-nodejs-modules.js
View file
Open in desktop
@@ -1,5 +1,5 @@
import importType from '../core/importType';
import isStaticRequire from '../core/staticRequire';
import moduleVisitor from 'eslint-module-utils/moduleVisitor';
import docsUrl from '../docsUrl';

function reportIfMissing(context, node, allowed, name) {
Expand Down Expand Up @@ -35,15 +35,8 @@ module.exports = {
const options = context.options[0] || {};
const allowed = options.allow || [];

return {
ImportDeclaration: function handleImports(node) {
reportIfMissing(context, node, allowed, node.source.value);
},
CallExpression: function handleRequires(node) {
if (isStaticRequire(node)) {
reportIfMissing(context, node, allowed, node.arguments[0].value);
}
},
};
return moduleVisitor((source, node) => {
reportIfMissing(context, node, allowed, source.value);
}, { commonjs: true });
},
};
17 changes: 4 additions & 13 deletions src/rules/no-restricted-paths.js
View file
Open in desktop
Expand Up @@ -2,7 +2,7 @@ import containsPath from 'contains-path';
import path from 'path';

import resolve from 'eslint-module-utils/resolve';
import isStaticRequire from '../core/staticRequire';
import moduleVisitor from 'eslint-module-utils/moduleVisitor';
import docsUrl from '../docsUrl';
import importType from '../core/importType';

Expand Down Expand Up @@ -109,17 +109,8 @@ module.exports = {
});
}

return {
ImportDeclaration(node) {
checkForRestrictedImportPath(node.source.value, node.source);
},
CallExpression(node) {
if (isStaticRequire(node)) {
const [ firstArgument ] = node.arguments;

checkForRestrictedImportPath(firstArgument.value, firstArgument);
}
},
};
return moduleVisitor((source) => {
checkForRestrictedImportPath(source.value, source);
}, { commonjs: true });
},
};
15 changes: 4 additions & 11 deletions src/rules/no-self-import.js
View file
Open in desktop
Expand Up @@ -4,7 +4,7 @@
*/

import resolve from 'eslint-module-utils/resolve';
import isStaticRequire from '../core/staticRequire';
import moduleVisitor from 'eslint-module-utils/moduleVisitor';
import docsUrl from '../docsUrl';

function isImportingSelf(context, node, requireName) {
Expand All @@ -31,15 +31,8 @@ module.exports = {
schema: [],
},
create: function (context) {
return {
ImportDeclaration(node) {
isImportingSelf(context, node, node.source.value);
},
CallExpression(node) {
if (isStaticRequire(node)) {
isImportingSelf(context, node, node.arguments[0].value);
}
},
};
return moduleVisitor((source, node) => {
isImportingSelf(context, node, source.value);
}, { commonjs: true });
},
};
15 changes: 4 additions & 11 deletions src/rules/no-webpack-loader-syntax.js
View file
Open in desktop
@@ -1,4 +1,4 @@
import isStaticRequire from '../core/staticRequire';
import moduleVisitor from 'eslint-module-utils/moduleVisitor';
import docsUrl from '../docsUrl';

function reportIfNonStandard(context, node, name) {
Expand All @@ -19,15 +19,8 @@ module.exports = {
},

create: function (context) {
return {
ImportDeclaration: function handleImports(node) {
reportIfNonStandard(context, node, node.source.value);
},
CallExpression: function handleRequires(node) {
if (isStaticRequire(node)) {
reportIfNonStandard(context, node, node.arguments[0].value);
}
},
};
return moduleVisitor((source, node) => {
reportIfNonStandard(context, node, source.value);
}, { commonjs: true });
},
};
93 changes: 93 additions & 0 deletions tests/src/rules/extensions.js
View file
Open in desktop
Expand Up @@ -391,6 +391,22 @@ ruleTester.run('extensions', rule, {
options: [ 'never', { ignorePackages: true } ],
}),

test({
code: `
import foo from './foo.js'
import bar from './bar.json'
import Component from './Component.jsx'
`,
errors: [
{
message: 'Unexpected use of file extension "jsx" for "./Component.jsx"',
line: 4,
column: 31,
},
],
options: [ 'always', { pattern: { jsx: 'never' } } ],
}),

// export (#964)
test({
code: [
Expand Down Expand Up @@ -444,6 +460,48 @@ ruleTester.run('extensions', rule, {
},
],
}),
// require (#1230)
test({
code: [
'const { foo } = require("./foo")',
'export { foo }',
].join('\n'),
options: [ 'always' ],
errors: [
{
message: 'Missing file extension for "./foo"',
line: 1,
column: 25,
},
],
}),
test({
code: [
'const { foo } = require("./foo.js")',
'export { foo }',
].join('\n'),
options: [ 'never' ],
errors: [
{
message: 'Unexpected use of file extension "js" for "./foo.js"',
line: 1,
column: 25,
},
],
}),

// export { } from
test({
code: 'export { foo } from "./foo"',
options: [ 'always' ],
errors: [
{
message: 'Missing file extension for "./foo"',
line: 1,
column: 21,
},
],
}),
test({
code: 'import foo from "@/ImNotAScopedModule"',
options: ['always'],
Expand All @@ -454,5 +512,40 @@ ruleTester.run('extensions', rule, {
},
],
}),
test({
code: 'export { foo } from "./foo.js"',
options: [ 'never' ],
errors: [
{
message: 'Unexpected use of file extension "js" for "./foo.js"',
line: 1,
column: 21,
},
],
}),

// export * from
test({
code: 'export * from "./foo"',
options: [ 'always' ],
errors: [
{
message: 'Missing file extension for "./foo"',
line: 1,
column: 15,
},
],
}),
test({
code: 'export * from "./foo.js"',
options: [ 'never' ],
errors: [
{
message: 'Unexpected use of file extension "js" for "./foo.js"',
line: 1,
column: 15,
},
],
}),
],
});