From df979d317571e7378b9072348c338efa16f28cc8 Mon Sep 17 00:00:00 2001 From: Toru Nagashima Date: Thu, 5 Sep 2019 10:08:58 +0900 Subject: [PATCH] =?UTF-8?q?=F0=9F=8E=A8=20refactor=20how=20to=20collect=20?= =?UTF-8?q?'require/import'?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- lib/rules/file-extension-in-import.js | 11 +-- lib/rules/no-extraneous-import.js | 14 +-- lib/rules/no-extraneous-require.js | 14 +-- lib/rules/no-hide-core-modules.js | 87 ++++++++++--------- lib/rules/no-missing-import.js | 10 +-- lib/rules/no-missing-require.js | 10 +-- lib/rules/no-unpublished-import.js | 14 +-- lib/rules/no-unpublished-require.js | 10 +-- .../no-unsupported-features/es-syntax.js | 38 +------- lib/util/get-require-targets.js | 48 ---------- lib/util/merge-visitors-in-place.js | 46 ++++++++++ ...port-export-targets.js => visit-import.js} | 41 ++++----- lib/util/visit-require.js | 59 +++++++++++++ 13 files changed, 204 insertions(+), 198 deletions(-) delete mode 100644 lib/util/get-require-targets.js create mode 100644 lib/util/merge-visitors-in-place.js rename lib/util/{get-import-export-targets.js => visit-import.js} (59%) create mode 100644 lib/util/visit-require.js diff --git a/lib/rules/file-extension-in-import.js b/lib/rules/file-extension-in-import.js index b8d41ac3..84f60586 100644 --- a/lib/rules/file-extension-in-import.js +++ b/lib/rules/file-extension-in-import.js @@ -6,8 +6,8 @@ const path = require("path") const fs = require("fs") -const getImportExportTargets = require("../util/get-import-export-targets") const getTryExtensions = require("../util/get-try-extensions") +const visitImport = require("../util/visit-import") const packageNamePattern = /^(?:@[^/\\]+[/\\])?[^/\\]+$/u const corePackageOverridePattern = /^(?:assert|async_hooks|buffer|child_process|cluster|console|constants|crypto|dgram|dns|domain|events|fs|http|http2|https|inspector|module|net|os|path|perf_hooks|process|punycode|querystring|readline|repl|stream|string_decoder|sys|timers|tls|trace_events|tty|url|util|v8|vm|worker_threads|zlib)[/\\]$/u @@ -122,11 +122,8 @@ module.exports = { } } - return { - "Program:exit"(node) { - const opts = { optionIndex: 1 } - getImportExportTargets(context, node, opts).forEach(verify) - }, - } + return visitImport(context, { optionIndex: 1 }, targets => { + targets.forEach(verify) + }) }, } diff --git a/lib/rules/no-extraneous-import.js b/lib/rules/no-extraneous-import.js index 515718d1..08a0840e 100644 --- a/lib/rules/no-extraneous-import.js +++ b/lib/rules/no-extraneous-import.js @@ -7,9 +7,9 @@ const checkExtraneous = require("../util/check-extraneous") const getAllowModules = require("../util/get-allow-modules") const getConvertPath = require("../util/get-convert-path") -const getImportTargets = require("../util/get-import-export-targets") const getResolvePaths = require("../util/get-resolve-paths") const getTryExtensions = require("../util/get-try-extensions") +const visitImport = require("../util/visit-import") module.exports = { meta: { @@ -42,14 +42,8 @@ module.exports = { return {} } - return { - "Program:exit"(node) { - checkExtraneous( - context, - filePath, - getImportTargets(context, node, false) - ) - }, - } + return visitImport(context, {}, targets => { + checkExtraneous(context, filePath, targets) + }) }, } diff --git a/lib/rules/no-extraneous-require.js b/lib/rules/no-extraneous-require.js index 7de40050..dff966af 100644 --- a/lib/rules/no-extraneous-require.js +++ b/lib/rules/no-extraneous-require.js @@ -7,9 +7,9 @@ const checkExtraneous = require("../util/check-extraneous") const getAllowModules = require("../util/get-allow-modules") const getConvertPath = require("../util/get-convert-path") -const getRequireTargets = require("../util/get-require-targets") const getResolvePaths = require("../util/get-resolve-paths") const getTryExtensions = require("../util/get-try-extensions") +const visitRequire = require("../util/visit-require") module.exports = { meta: { @@ -42,14 +42,8 @@ module.exports = { return {} } - return { - "Program:exit"() { - checkExtraneous( - context, - filePath, - getRequireTargets(context, false) - ) - }, - } + return visitRequire(context, {}, targets => { + checkExtraneous(context, filePath, targets) + }) }, } diff --git a/lib/rules/no-hide-core-modules.js b/lib/rules/no-hide-core-modules.js index d80c37e7..6f26c372 100644 --- a/lib/rules/no-hide-core-modules.js +++ b/lib/rules/no-hide-core-modules.js @@ -11,8 +11,9 @@ const path = require("path") const resolve = require("resolve") const getPackageJson = require("../util/get-package-json") -const getRequireTargets = require("../util/get-require-targets") -const getImportExportTargets = require("../util/get-import-export-targets") +const mergeVisitorsInPlace = require("../util/merge-visitors-in-place") +const visitImport = require("../util/visit-import") +const visitRequire = require("../util/visit-require") const CORE_MODULES = new Set([ "assert", @@ -98,49 +99,57 @@ module.exports = { const ignoreIndirectDependencies = Boolean( options.ignoreIndirectDependencies ) + const targets = [] - return { - "Program:exit"(node) { - const targets = [] - .concat( - getRequireTargets(context, true), - getImportExportTargets(context, node, { - includeCore: true, - }) - ) - .filter(t => CORE_MODULES.has(t.moduleName)) + return [ + visitImport(context, { includeCore: true }, importTargets => + targets.push(...importTargets) + ), + visitRequire(context, { includeCore: true }, requireTargets => + targets.push(...requireTargets) + ), + { + "Program:exit"() { + for (const target of targets.filter(t => + CORE_MODULES.has(t.moduleName) + )) { + const name = target.moduleName + const allowed = + allow.indexOf(name) !== -1 || + (ignoreDirectDependencies && deps.has(name)) || + (ignoreIndirectDependencies && !deps.has(name)) - for (const target of targets) { - const name = target.moduleName - const allowed = - allow.indexOf(name) !== -1 || - (ignoreDirectDependencies && deps.has(name)) || - (ignoreIndirectDependencies && !deps.has(name)) + if (allowed) { + continue + } - if (allowed) { - continue - } + const resolved = resolve.sync(name, { + basedir: dirPath, + }) + const isCore = resolved === name - const resolved = resolve.sync(name, { basedir: dirPath }) - const isCore = resolved === name + if (isCore) { + continue + } - if (isCore) { - continue + context.report({ + node: target.node, + loc: target.node.loc, + message: + "Unexpected import of third-party module '{{name}}'.", + data: { + name: path + .relative(dirPath, resolved) + .replace(BACK_SLASH, "/"), + }, + }) } - - context.report({ - node: target.node, - loc: target.node.loc, - message: - "Unexpected import of third-party module '{{name}}'.", - data: { - name: path - .relative(dirPath, resolved) - .replace(BACK_SLASH, "/"), - }, - }) - } + }, }, - } + ].reduce( + (mergedVisitor, thisVisitor) => + mergeVisitorsInPlace(mergedVisitor, thisVisitor), + {} + ) }, } diff --git a/lib/rules/no-missing-import.js b/lib/rules/no-missing-import.js index 72cb875f..e954e8fa 100644 --- a/lib/rules/no-missing-import.js +++ b/lib/rules/no-missing-import.js @@ -6,9 +6,9 @@ const checkExistence = require("../util/check-existence") const getAllowModules = require("../util/get-allow-modules") -const getImportExportTargets = require("../util/get-import-export-targets") const getResolvePaths = require("../util/get-resolve-paths") const getTryExtensions = require("../util/get-try-extensions") +const visitImport = require("../util/visit-import") module.exports = { meta: { @@ -40,10 +40,8 @@ module.exports = { return {} } - return { - "Program:exit"(node) { - checkExistence(context, getImportExportTargets(context, node)) - }, - } + return visitImport(context, {}, targets => { + checkExistence(context, targets) + }) }, } diff --git a/lib/rules/no-missing-require.js b/lib/rules/no-missing-require.js index a26a03af..67d301ba 100644 --- a/lib/rules/no-missing-require.js +++ b/lib/rules/no-missing-require.js @@ -6,9 +6,9 @@ const checkExistence = require("../util/check-existence") const getAllowModules = require("../util/get-allow-modules") -const getRequireTargets = require("../util/get-require-targets") const getResolvePaths = require("../util/get-resolve-paths") const getTryExtensions = require("../util/get-try-extensions") +const visitRequire = require("../util/visit-require") module.exports = { meta: { @@ -40,10 +40,8 @@ module.exports = { return {} } - return { - "Program:exit"() { - checkExistence(context, getRequireTargets(context)) - }, - } + return visitRequire(context, {}, targets => { + checkExistence(context, targets) + }) }, } diff --git a/lib/rules/no-unpublished-import.js b/lib/rules/no-unpublished-import.js index b3680a3e..152a6a7d 100644 --- a/lib/rules/no-unpublished-import.js +++ b/lib/rules/no-unpublished-import.js @@ -7,9 +7,9 @@ const checkPublish = require("../util/check-publish") const getAllowModules = require("../util/get-allow-modules") const getConvertPath = require("../util/get-convert-path") -const getImportExportTargets = require("../util/get-import-export-targets") const getResolvePaths = require("../util/get-resolve-paths") const getTryExtensions = require("../util/get-try-extensions") +const visitImport = require("../util/visit-import") module.exports = { meta: { @@ -42,14 +42,8 @@ module.exports = { return {} } - return { - "Program:exit"(node) { - checkPublish( - context, - filePath, - getImportExportTargets(context, node) - ) - }, - } + return visitImport(context, {}, targets => { + checkPublish(context, filePath, targets) + }) }, } diff --git a/lib/rules/no-unpublished-require.js b/lib/rules/no-unpublished-require.js index 81251b99..9b3b1bc4 100644 --- a/lib/rules/no-unpublished-require.js +++ b/lib/rules/no-unpublished-require.js @@ -7,9 +7,9 @@ const checkPublish = require("../util/check-publish") const getAllowModules = require("../util/get-allow-modules") const getConvertPath = require("../util/get-convert-path") -const getRequireTargets = require("../util/get-require-targets") const getResolvePaths = require("../util/get-resolve-paths") const getTryExtensions = require("../util/get-try-extensions") +const visitRequire = require("../util/visit-require") module.exports = { meta: { @@ -42,10 +42,8 @@ module.exports = { return {} } - return { - "Program:exit"() { - checkPublish(context, filePath, getRequireTargets(context)) - }, - } + return visitRequire(context, {}, targets => { + checkPublish(context, filePath, targets) + }) }, } diff --git a/lib/rules/no-unsupported-features/es-syntax.js b/lib/rules/no-unsupported-features/es-syntax.js index e4057212..e1fb5945 100644 --- a/lib/rules/no-unsupported-features/es-syntax.js +++ b/lib/rules/no-unsupported-features/es-syntax.js @@ -9,6 +9,7 @@ const { getInnermostScope } = require("eslint-utils") const { Range } = require("semver") //eslint-disable-line no-unused-vars const getConfiguredNodeVersion = require("../../util/get-configured-node-version") const getSemverRange = require("../../util/get-semver-range") +const mergeVisitorsInPlace = require("../../util/merge-visitors-in-place") const getOrSet = /^(?:g|s)et$/u const features = { @@ -405,41 +406,6 @@ function normalizeScope(initialScope, node) { return scope } -/** - * Merge two visitors. - * @param {Visitor} x The visitor which is assigned. - * @param {Visitor} y The visitor which is assigning. - * @returns {Visitor} `x`. - */ -function merge(x, y) { - for (const key of Object.keys(y)) { - if (typeof x[key] === "function") { - if (x[key]._handlers == null) { - const fs = [x[key], y[key]] - x[key] = dispatch.bind(null, fs) - x[key]._handlers = fs - } else { - x[key]._handlers.push(y[key]) - } - } else { - x[key] = y[key] - } - } - return x -} - -/** - * Dispatch all given functions with a node. - * @param {function[]} handlers The function list to call. - * @param {Node} node The AST node to be handled. - * @returns {void} - */ -function dispatch(handlers, node) { - for (const h of handlers) { - h(node) - } -} - /** * Define the visitor object as merging the rules of eslint-plugin-es. * @param {RuleContext} context The rule context. @@ -514,7 +480,7 @@ function defineVisitor(context, options) { } }, } - return merge(visitor, rule.create(thisContext)) + return mergeVisitorsInPlace(visitor, rule.create(thisContext)) }, {}) ) } diff --git a/lib/util/get-require-targets.js b/lib/util/get-require-targets.js deleted file mode 100644 index 4da2c70b..00000000 --- a/lib/util/get-require-targets.js +++ /dev/null @@ -1,48 +0,0 @@ -/** - * @author Toru Nagashima - * See LICENSE file in root directory for full license. - */ -"use strict" - -const path = require("path") -const { CALL, ReferenceTracker, getStringIfConstant } = require("eslint-utils") -const resolve = require("resolve") -const getResolvePaths = require("./get-resolve-paths") -const getTryExtensions = require("./get-try-extensions") -const ImportTarget = require("./import-target") -const stripImportPathParams = require("./strip-import-path-params") - -/** - * Gets a list of `require()` targets. - * - * Core modules of Node.js (e.g. `fs`, `http`) are excluded. - * - * @param {RuleContext} context - The rule context. - * @param {boolean} includeCore - The flag to include core modules. - * @returns {ImportTarget[]} A list of found target's information. - */ -module.exports = function getRequireTargets(context, includeCore) { - const retv = [] - const basedir = path.dirname(path.resolve(context.getFilename())) - const paths = getResolvePaths(context) - const extensions = getTryExtensions(context) - const options = { basedir, paths, extensions } - const tracker = new ReferenceTracker(context.getScope()) - const references = tracker.iterateGlobalReferences({ - require: { - [CALL]: true, - resolve: { [CALL]: true }, - }, - }) - - for (const { node } of references) { - const targetNode = node.arguments[0] - const rawName = getStringIfConstant(targetNode) - const name = rawName && stripImportPathParams(rawName) - if (name && (includeCore || !resolve.isCore(name))) { - retv.push(new ImportTarget(targetNode, name, options)) - } - } - - return retv -} diff --git a/lib/util/merge-visitors-in-place.js b/lib/util/merge-visitors-in-place.js new file mode 100644 index 00000000..eea30da3 --- /dev/null +++ b/lib/util/merge-visitors-in-place.js @@ -0,0 +1,46 @@ +/** + * @author Toru Nagashima + * See LICENSE file in root directory for full license. + */ +"use strict" + +/** + * Merge two visitors. + * This function modifies `visitor1` directly to merge. + * @param {Visitor} visitor1 The visitor which is assigned. + * @param {Visitor} visitor2 The visitor which is assigning. + * @returns {Visitor} `visitor1`. + */ +module.exports = function mergeVisitorsInPlace(visitor1, visitor2) { + for (const key of Object.keys(visitor2)) { + const handler1 = visitor1[key] + const handler2 = visitor2[key] + + if (typeof handler1 === "function") { + if (handler1._handlers) { + handler1._handlers.push(handler2) + } else { + const handlers = [handler1, handler2] + visitor1[key] = Object.assign(dispatch.bind(null, handlers), { + _handlers: handlers, + }) + } + } else { + visitor1[key] = handler2 + } + } + + return visitor1 +} + +/** + * Dispatch all given functions with a node. + * @param {function[]} handlers The function list to call. + * @param {Node} node The AST node to be handled. + * @returns {void} + */ +function dispatch(handlers, node) { + for (const h of handlers) { + h(node) + } +} diff --git a/lib/util/get-import-export-targets.js b/lib/util/visit-import.js similarity index 59% rename from lib/util/get-import-export-targets.js rename to lib/util/visit-import.js index 08ac36f2..c44e2fe6 100644 --- a/lib/util/get-import-export-targets.js +++ b/lib/util/visit-import.js @@ -11,44 +11,45 @@ const getTryExtensions = require("./get-try-extensions") const ImportTarget = require("./import-target") const stripImportPathParams = require("./strip-import-path-params") -const MODULE_TYPE = /^(?:Import|Export(?:Named|Default|All))Declaration$/u - /** * Gets a list of `import`/`export` declaration targets. * * Core modules of Node.js (e.g. `fs`, `http`) are excluded. * * @param {RuleContext} context - The rule context. - * @param {ASTNode} programNode - The node of Program. * @param {Object} [options] - The flag to include core modules. * @param {boolean} [options.includeCore] - The flag to include core modules. * @param {number} [options.optionIndex] - The index of rule options. + * @param {function(ImportTarget[]):void} callback The callback function to get result. * @returns {ImportTarget[]} A list of found target's information. */ -module.exports = function getImportExportTargets( +module.exports = function visitImport( context, - programNode, - { includeCore = false, optionIndex = 0 } = {} + { includeCore = false, optionIndex = 0 } = {}, + callback ) { - const retv = [] + const targets = [] const basedir = path.dirname(path.resolve(context.getFilename())) const paths = getResolvePaths(context, optionIndex) const extensions = getTryExtensions(context, optionIndex) const options = { basedir, paths, extensions } - for (const statement of programNode.body) { - // Skip if it's not a module declaration. - if (!MODULE_TYPE.test(statement.type)) { - continue - } + return { + [[ + "ExportAllDeclaration", + "ExportDefaultDeclaration", + "ExportNamedDeclaration", + "ImportDeclaration", + ]](node) { + const sourceNode = node.source + const name = sourceNode && stripImportPathParams(sourceNode.value) + if (name && (includeCore || !resolve.isCore(name))) { + targets.push(new ImportTarget(sourceNode, name, options)) + } + }, - // Gets the target module. - const node = statement.source - const name = node && stripImportPathParams(node.value) - if (name && (includeCore || !resolve.isCore(name))) { - retv.push(new ImportTarget(node, name, options)) - } + "Program:exit"() { + callback(targets) + }, } - - return retv } diff --git a/lib/util/visit-require.js b/lib/util/visit-require.js new file mode 100644 index 00000000..23c92aa7 --- /dev/null +++ b/lib/util/visit-require.js @@ -0,0 +1,59 @@ +/** + * @author Toru Nagashima + * See LICENSE file in root directory for full license. + */ +"use strict" + +const path = require("path") +const { CALL, ReferenceTracker, getStringIfConstant } = require("eslint-utils") +const resolve = require("resolve") +const getResolvePaths = require("./get-resolve-paths") +const getTryExtensions = require("./get-try-extensions") +const ImportTarget = require("./import-target") +const stripImportPathParams = require("./strip-import-path-params") + +/** + * Gets a list of `require()` targets. + * + * Core modules of Node.js (e.g. `fs`, `http`) are excluded. + * + * @param {RuleContext} context - The rule context. + * @param {Object} [options] - The flag to include core modules. + * @param {boolean} [options.includeCore] - The flag to include core modules. + * @param {function(ImportTarget[]):void} callback The callback function to get result. + * @returns {Object} The visitor. + */ +module.exports = function visitRequire( + context, + { includeCore = false } = {}, + callback +) { + const targets = [] + const basedir = path.dirname(path.resolve(context.getFilename())) + const paths = getResolvePaths(context) + const extensions = getTryExtensions(context) + const options = { basedir, paths, extensions } + + return { + "Program:exit"() { + const tracker = new ReferenceTracker(context.getScope()) + const references = tracker.iterateGlobalReferences({ + require: { + [CALL]: true, + resolve: { [CALL]: true }, + }, + }) + + for (const { node } of references) { + const targetNode = node.arguments[0] + const rawName = getStringIfConstant(targetNode) + const name = rawName && stripImportPathParams(rawName) + if (name && (includeCore || !resolve.isCore(name))) { + targets.push(new ImportTarget(targetNode, name, options)) + } + } + + callback(targets) + }, + } +}