From e5edf49ecdc745a947ef7931b5dee0ef858d6b93 Mon Sep 17 00:00:00 2001 From: michael faith Date: Sun, 15 Dec 2024 09:06:33 -0600 Subject: [PATCH] [Fix] `no-unused-modules`: provide more meaningful error message when no eslintrc is present This change adjusts what we're doing if an error is thrown while attempting to enumerate lintable files in the no-used-modules rule, when users are running with flat config. Now, if FileEnumerator throws due to a lack of eslintrc and the user is running with flat config, we catch the error and rethrow with a more informative message. I also cleaned up the original aspects of the implementation that was using the proposed eslint context functions, since that proposal was walked back and the API was never introduced. Note: This isn't an ideal state, since this rule still relies on the legacy api to understand what needs to be ignored. Remaining tethered to the legacy config system is going to need to be solved at some point. Fixes #3079 --- CHANGELOG.md | 4 + examples/v9/eslint.config.mjs | 22 +++++ examples/v9/package.json | 14 +++ examples/v9/src/depth-zero.js | 3 + examples/v9/src/es6/depth-one-dynamic.js | 3 + examples/v9/src/exports-unused.js | 6 ++ examples/v9/src/exports.js | 6 ++ examples/v9/src/imports.js | 6 ++ package.json | 4 +- src/core/fsWalk.js | 48 --------- src/rules/no-unused-modules.js | 118 ++++++++++------------- tests/src/rules/no-unused-modules.js | 39 ++++++++ 12 files changed, 157 insertions(+), 116 deletions(-) create mode 100644 examples/v9/eslint.config.mjs create mode 100644 examples/v9/package.json create mode 100644 examples/v9/src/depth-zero.js create mode 100644 examples/v9/src/es6/depth-one-dynamic.js create mode 100644 examples/v9/src/exports-unused.js create mode 100644 examples/v9/src/exports.js create mode 100644 examples/v9/src/imports.js delete mode 100644 src/core/fsWalk.js diff --git a/CHANGELOG.md b/CHANGELOG.md index 3a57da91fb..5bcd7163ff 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,9 @@ This change log adheres to standards from [Keep a CHANGELOG](https://keepachange - add [`enforce-node-protocol-usage`] rule and `import/node-version` setting ([#3024], thanks [@GoldStrikeArch] and [@sevenc-nanashi]) - add TypeScript types ([#3097], thanks [@G-Rath]) +### Fixed +- [`no-unused-modules`]: provide more meaningful error message when no .eslintrc is present ([#3116], thanks [@michaelfaith]) + ### Changed - [Docs] [`extensions`], [`order`]: improve documentation ([#3106], thanks [@Xunnamius]) - [Docs] add flat config guide for using `tseslint.config()` ([#3125], thanks [@lnuvy]) @@ -1165,6 +1168,7 @@ for info on changes for earlier releases. [#3125]: https://github.com/import-js/eslint-plugin-import/pull/3125 [#3122]: https://github.com/import-js/eslint-plugin-import/pull/3122 +[#3116]: https://github.com/import-js/eslint-plugin-import/pull/3116 [#3106]: https://github.com/import-js/eslint-plugin-import/pull/3106 [#3097]: https://github.com/import-js/eslint-plugin-import/pull/3097 [#3073]: https://github.com/import-js/eslint-plugin-import/pull/3073 diff --git a/examples/v9/eslint.config.mjs b/examples/v9/eslint.config.mjs new file mode 100644 index 0000000000..7b7534d140 --- /dev/null +++ b/examples/v9/eslint.config.mjs @@ -0,0 +1,22 @@ +import importPlugin from 'eslint-plugin-import'; +import js from '@eslint/js'; + +export default [ + js.configs.recommended, + importPlugin.flatConfigs.recommended, + { + files: ['**/*.{js,mjs,cjs}'], + languageOptions: { + ecmaVersion: 'latest', + sourceType: 'module', + }, + ignores: ['eslint.config.mjs', 'node_modules/*'], + rules: { + 'no-unused-vars': 'off', + 'import/no-dynamic-require': 'warn', + 'import/no-nodejs-modules': 'warn', + 'import/no-unused-modules': ['warn', { unusedExports: true }], + 'import/no-cycle': 'warn', + }, + }, +]; diff --git a/examples/v9/package.json b/examples/v9/package.json new file mode 100644 index 0000000000..4746d74005 --- /dev/null +++ b/examples/v9/package.json @@ -0,0 +1,14 @@ +{ + "name": "v9", + "version": "1.0.0", + "main": "index.js", + "type": "module", + "scripts": { + "lint": "eslint src --report-unused-disable-directives" + }, + "devDependencies": { + "@eslint/js": "^9.17.0", + "eslint": "^9.17.0", + "eslint-plugin-import": "file:../.." + } +} diff --git a/examples/v9/src/depth-zero.js b/examples/v9/src/depth-zero.js new file mode 100644 index 0000000000..8cfde99795 --- /dev/null +++ b/examples/v9/src/depth-zero.js @@ -0,0 +1,3 @@ +import { foo } from "./es6/depth-one-dynamic"; + +foo(); diff --git a/examples/v9/src/es6/depth-one-dynamic.js b/examples/v9/src/es6/depth-one-dynamic.js new file mode 100644 index 0000000000..ca129fd622 --- /dev/null +++ b/examples/v9/src/es6/depth-one-dynamic.js @@ -0,0 +1,3 @@ +export function foo() {} + +export const bar = () => import("../depth-zero").then(({foo}) => foo); diff --git a/examples/v9/src/exports-unused.js b/examples/v9/src/exports-unused.js new file mode 100644 index 0000000000..3c44db68d0 --- /dev/null +++ b/examples/v9/src/exports-unused.js @@ -0,0 +1,6 @@ +export const a = 13; +export const b = 18; + +const defaultExport = { a, b }; + +export default defaultExport; diff --git a/examples/v9/src/exports.js b/examples/v9/src/exports.js new file mode 100644 index 0000000000..3c44db68d0 --- /dev/null +++ b/examples/v9/src/exports.js @@ -0,0 +1,6 @@ +export const a = 13; +export const b = 18; + +const defaultExport = { a, b }; + +export default defaultExport; diff --git a/examples/v9/src/imports.js b/examples/v9/src/imports.js new file mode 100644 index 0000000000..edf1336865 --- /dev/null +++ b/examples/v9/src/imports.js @@ -0,0 +1,6 @@ +//import c from './exports'; +import { a, b } from './exports'; + +import path from 'path'; +import fs from 'node:fs'; +import console from 'console'; diff --git a/package.json b/package.json index f3053e1c11..268b477ea4 100644 --- a/package.json +++ b/package.json @@ -33,9 +33,10 @@ "test": "npm run tests-only", "test-compiled": "npm run prepublish && BABEL_ENV=testCompiled mocha --compilers js:babel-register tests/src", "test-all": "node --require babel-register ./scripts/testAll", - "test-examples": "npm run build && npm run test-example:legacy && npm run test-example:flat", + "test-examples": "npm run build && npm run test-example:legacy && npm run test-example:flat && npm run test-example:v9", "test-example:legacy": "cd examples/legacy && npm install && npm run lint", "test-example:flat": "cd examples/flat && npm install && npm run lint", + "test-example:v9": "cd examples/v9 && npm install && npm run lint", "test-types": "npx --package typescript@latest tsc --noEmit index.d.ts", "prepublishOnly": "safe-publish-latest && npm run build", "prepublish": "not-in-publish || npm run prepublishOnly", @@ -106,6 +107,7 @@ "rimraf": "^2.7.1", "safe-publish-latest": "^2.0.0", "sinon": "^2.4.1", + "tmp": "^0.2.1", "typescript": "^2.8.1 || ~3.9.5 || ~4.5.2", "typescript-eslint-parser": "^15 || ^20 || ^22" }, diff --git a/src/core/fsWalk.js b/src/core/fsWalk.js deleted file mode 100644 index fa112590f1..0000000000 --- a/src/core/fsWalk.js +++ /dev/null @@ -1,48 +0,0 @@ -/** - * This is intended to provide similar capability as the sync api from @nodelib/fs.walk, until `eslint-plugin-import` - * is willing to modernize and update their minimum node version to at least v16. I intentionally made the - * shape of the API (for the part we're using) the same as @nodelib/fs.walk so that that can be swapped in - * when the repo is ready for it. - */ - -import path from 'path'; -import { readdirSync } from 'fs'; - -/** @typedef {{ name: string, path: string, dirent: import('fs').Dirent }} Entry */ - -/** - * Do a comprehensive walk of the provided src directory, and collect all entries. Filter out - * any directories or entries using the optional filter functions. - * @param {string} root - path to the root of the folder we're walking - * @param {{ deepFilter?: (entry: Entry) => boolean, entryFilter?: (entry: Entry) => boolean }} options - * @param {Entry} currentEntry - entry for the current directory we're working in - * @param {Entry[]} existingEntries - list of all entries so far - * @returns {Entry[]} an array of directory entries - */ -export function walkSync(root, options, currentEntry, existingEntries) { - // Extract the filter functions. Default to evaluating true, if no filter passed in. - const { deepFilter = () => true, entryFilter = () => true } = options; - - let entryList = existingEntries || []; - const currentRelativePath = currentEntry ? currentEntry.path : '.'; - const fullPath = currentEntry ? path.join(root, currentEntry.path) : root; - - const dirents = readdirSync(fullPath, { withFileTypes: true }); - dirents.forEach((dirent) => { - /** @type {Entry} */ - const entry = { - name: dirent.name, - path: path.join(currentRelativePath, dirent.name), - dirent, - }; - - if (dirent.isDirectory() && deepFilter(entry)) { - entryList.push(entry); - entryList = walkSync(root, options, entry, entryList); - } else if (dirent.isFile() && entryFilter(entry)) { - entryList.push(entry); - } - }); - - return entryList; -} diff --git a/src/rules/no-unused-modules.js b/src/rules/no-unused-modules.js index 358726299d..86302a0ea6 100644 --- a/src/rules/no-unused-modules.js +++ b/src/rules/no-unused-modules.js @@ -8,13 +8,12 @@ import { getPhysicalFilename } from 'eslint-module-utils/contextCompat'; import { getFileExtensions } from 'eslint-module-utils/ignore'; import resolve from 'eslint-module-utils/resolve'; import visit from 'eslint-module-utils/visit'; -import { dirname, join, resolve as resolvePath } from 'path'; +import { dirname, join } from 'path'; import readPkgUp from 'eslint-module-utils/readPkgUp'; import values from 'object.values'; import includes from 'array-includes'; import flatMap from 'array.prototype.flatmap'; -import { walkSync } from '../core/fsWalk'; import ExportMapBuilder from '../exportMap/builder'; import recursivePatternCapture from '../exportMap/patternCapture'; import docsUrl from '../docsUrl'; @@ -51,21 +50,62 @@ function requireFileEnumerator() { } /** - * + * Given a FileEnumerator class, instantiate and load the list of files. * @param FileEnumerator the `FileEnumerator` class from `eslint`'s internal api * @param {string} src path to the src root * @param {string[]} extensions list of supported extensions * @returns {{ filename: string, ignored: boolean }[]} list of files to operate on */ function listFilesUsingFileEnumerator(FileEnumerator, src, extensions) { - const e = new FileEnumerator({ + // We need to know whether this is being run with flat config in order to + // determine how to report errors if FileEnumerator throws due to a lack of eslintrc. + + const { ESLINT_USE_FLAT_CONFIG } = process.env; + + // This condition is sufficient to test in v8, since the environment variable is necessary to turn on flat config + let isUsingFlatConfig = ESLINT_USE_FLAT_CONFIG && process.env.ESLINT_USE_FLAT_CONFIG !== 'false'; + + // In the case of using v9, we can check the `shouldUseFlatConfig` function + // If this function is present, then we assume it's v9 + try { + const { shouldUseFlatConfig } = require('eslint/use-at-your-own-risk'); + isUsingFlatConfig = shouldUseFlatConfig && ESLINT_USE_FLAT_CONFIG !== 'false'; + } catch (_) { + // We don't want to throw here, since we only want to update the + // boolean if the function is available. + } + + const enumerator = new FileEnumerator({ extensions, }); - return Array.from( - e.iterateFiles(src), - ({ filePath, ignored }) => ({ filename: filePath, ignored }), - ); + try { + return Array.from( + enumerator.iterateFiles(src), + ({ filePath, ignored }) => ({ filename: filePath, ignored }), + ); + } catch (e) { + // If we're using flat config, and FileEnumerator throws due to a lack of eslintrc, + // then we want to throw an error so that the user knows about this rule's reliance on + // the legacy config. + if ( + isUsingFlatConfig + && e.message.includes('No ESLint configuration found') + ) { + throw new Error(` +Due to the exclusion of certain internal ESLint APIs when using flat config, +the import/no-unused-modules rule requires an .eslintrc file to know which +files to ignore (even when using flat config). +The .eslintrc file only needs to contain "ignorePatterns", or can be empty if +you do not want to ignore any files. + +See https://github.com/import-js/eslint-plugin-import/issues/3079 +for additional context. +`); + } + // If this isn't the case, then we'll just let the error bubble up + throw e; + } } /** @@ -107,70 +147,14 @@ function listFilesWithLegacyFunctions(src, extensions) { } } -/** - * Given a source root and list of supported extensions, use fsWalk and the - * new `eslint` `context.session` api to build the list of files we want to operate on - * @param {string[]} srcPaths array of source paths (for flat config this should just be a singular root (e.g. cwd)) - * @param {string[]} extensions list of supported extensions - * @param {{ isDirectoryIgnored: (path: string) => boolean, isFileIgnored: (path: string) => boolean }} session eslint context session object - * @returns {string[]} list of files to operate on - */ -function listFilesWithModernApi(srcPaths, extensions, session) { - /** @type {string[]} */ - const files = []; - - for (let i = 0; i < srcPaths.length; i++) { - const src = srcPaths[i]; - // Use walkSync along with the new session api to gather the list of files - const entries = walkSync(src, { - deepFilter(entry) { - const fullEntryPath = resolvePath(src, entry.path); - - // Include the directory if it's not marked as ignore by eslint - return !session.isDirectoryIgnored(fullEntryPath); - }, - entryFilter(entry) { - const fullEntryPath = resolvePath(src, entry.path); - - // Include the file if it's not marked as ignore by eslint and its extension is included in our list - return ( - !session.isFileIgnored(fullEntryPath) - && extensions.find((extension) => entry.path.endsWith(extension)) - ); - }, - }); - - // Filter out directories and map entries to their paths - files.push( - ...entries - .filter((entry) => !entry.dirent.isDirectory()) - .map((entry) => entry.path), - ); - } - return files; -} - /** * Given a src pattern and list of supported extensions, return a list of files to process * with this rule. * @param {string} src - file, directory, or glob pattern of files to act on * @param {string[]} extensions - list of supported file extensions - * @param {import('eslint').Rule.RuleContext} context - the eslint context object * @returns {string[] | { filename: string, ignored: boolean }[]} the list of files that this rule will evaluate. */ -function listFilesToProcess(src, extensions, context) { - // If the context object has the new session functions, then prefer those - // Otherwise, fallback to using the deprecated `FileEnumerator` for legacy support. - // https://github.com/eslint/eslint/issues/18087 - if ( - context.session - && context.session.isFileIgnored - && context.session.isDirectoryIgnored - ) { - return listFilesWithModernApi(src, extensions, context.session); - } - - // Fallback to og FileEnumerator +function listFilesToProcess(src, extensions) { const FileEnumerator = requireFileEnumerator(); // If we got the FileEnumerator, then let's go with that @@ -295,10 +279,10 @@ const isNodeModule = (path) => (/\/(node_modules)\//).test(path); function resolveFiles(src, ignoreExports, context) { const extensions = Array.from(getFileExtensions(context.settings)); - const srcFileList = listFilesToProcess(src, extensions, context); + const srcFileList = listFilesToProcess(src, extensions); // prepare list of ignored files - const ignoredFilesList = listFilesToProcess(ignoreExports, extensions, context); + const ignoredFilesList = listFilesToProcess(ignoreExports, extensions); // The modern api will return a list of file paths, rather than an object if (ignoredFilesList.length && typeof ignoredFilesList[0] === 'string') { diff --git a/tests/src/rules/no-unused-modules.js b/tests/src/rules/no-unused-modules.js index d86f406220..a15d2c2376 100644 --- a/tests/src/rules/no-unused-modules.js +++ b/tests/src/rules/no-unused-modules.js @@ -3,8 +3,12 @@ import jsxConfig from '../../../config/react'; import typescriptConfig from '../../../config/typescript'; import { RuleTester } from '../rule-tester'; +import { expect } from 'chai'; +import { execSync } from 'child_process'; import fs from 'fs'; import eslintPkg from 'eslint/package.json'; +import path from 'path'; +import process from 'process'; import semver from 'semver'; let FlatRuleTester; @@ -14,6 +18,7 @@ try { // TODO: figure out why these tests fail in eslint 4 and 5 const isESLint4TODO = semver.satisfies(eslintPkg.version, '^4 || ^5'); +const isESLint9 = semver.satisfies(eslintPkg.version, '>=9'); const ruleTester = new RuleTester(); const typescriptRuleTester = new RuleTester(typescriptConfig); @@ -1482,3 +1487,37 @@ describe('parser ignores prefixes like BOM and hashbang', () => { }); }); }); + +(isESLint9 ? describe : describe.skip)('with eslint 9+', () => { + it('provides meaningful error when eslintrc is not present', () => { + const tmp = require('tmp'); + + // Create temp directory outside of project root + const tempDir = tmp.dirSync({ unsafeCleanup: true }); + + // Copy example project to temp directory + fs.cpSync(path.join(process.cwd(), 'examples/v9'), tempDir.name, { recursive: true }); + + let errorMessage = ''; + + // Build the plugin + try { + execSync('npm run build'); + } catch (_) { + /* ignore */ + } + + // Install the plugin and run the lint command in the temp directory + try { + execSync(`npm install -D ${process.cwd()} && npm run lint`, { cwd: tempDir.name }); + } catch (error) { + errorMessage = error.stderr.toString(); + } + + // Verify that the error message is as expected + expect(errorMessage).to.contain('the import/no-unused-modules rule requires an .eslintrc file'); + + // Cleanup + tempDir.removeCallback(); + }).timeout(100000); +});