Skip to content

Commit

Permalink
Simplify lintFiles (#583)
Browse files Browse the repository at this point in the history
  • Loading branch information
@fisker
fisker authored Aug 6, 2021
1 parent 2670d3d commit e2e715d
Show file tree
Hide file tree
Showing 4 changed files with 33 additions and 251 deletions.
57 changes: 30 additions & 27 deletions index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,21 +2,16 @@ import process from 'node:process';
import path from 'node:path';
import {ESLint} from 'eslint';
import {globby, isGitIgnoredSync} from 'globby';
import {isEqual} from 'lodash-es';
import {omit, isEqual} from 'lodash-es';
import micromatch from 'micromatch';
import arrify from 'arrify';
import pReduce from 'p-reduce';
import pMap from 'p-map';
import {cosmiconfig, defaultLoaders} from 'cosmiconfig';
import defineLazyProperty from 'define-lazy-prop';
import pFilter from 'p-filter';
import slash from 'slash';
import {CONFIG_FILES, MODULE_NAME, DEFAULT_IGNORES} from './lib/constants.js';
import {
normalizeOptions,
getIgnores,
mergeWithFileConfig,
mergeWithFileConfigs,
buildConfig,
mergeOptions,
} from './lib/options-manager.js';
Expand Down Expand Up @@ -87,10 +82,19 @@ const processReport = (report, {isQuiet = false} = {}) => {
return result;
};

const runEslint = async (paths, options, processorOptions) => {
const engine = new ESLint(options);
const runEslint = async (filePath, options, processorOptions) => {
const engine = new ESLint(omit(options, ['filePath', 'warnIgnored']));
const filename = path.relative(options.cwd, filePath);

const report = await engine.lintFiles(await pFilter(paths, async path => !(await engine.isPathIgnored(path))));
if (
micromatch.isMatch(filename, options.baseConfig.ignorePatterns)
|| isGitIgnoredSync({cwd: options.cwd, ignore: options.baseConfig.ignorePatterns})(filePath)
|| await engine.isPathIgnored(filePath)
) {
return;
}

const report = await engine.lintFiles([filePath]);
return processReport(report, processorOptions);
};

Expand Down Expand Up @@ -145,25 +149,24 @@ const lintText = async (string, inputOptions = {}) => {
};

const lintFiles = async (patterns, inputOptions = {}) => {
inputOptions = normalizeOptions(inputOptions);
inputOptions.cwd = path.resolve(inputOptions.cwd || process.cwd());
const configExplorer = cosmiconfig(MODULE_NAME, {searchPlaces: CONFIG_FILES, loaders: {noExt: defaultLoaders['.json']}, stopDir: inputOptions.cwd});

const configFiles = (await Promise.all(
(await globby(
CONFIG_FILES.map(configFile => `**/${configFile}`),
{ignore: DEFAULT_IGNORES, gitignore: true, absolute: true, cwd: inputOptions.cwd},
)).map(configFile => configExplorer.load(configFile)),
)).filter(Boolean);

const paths = configFiles.length > 0
? await pReduce(
configFiles,
async (paths, {filepath, config}) =>
[...paths, ...(await globFiles(patterns, {...mergeOptions(inputOptions, config), cwd: path.dirname(filepath)}))],
[])
: await globFiles(patterns, mergeOptions(inputOptions));

return mergeReports(await pMap(await mergeWithFileConfigs([...new Set(paths)], inputOptions, configFiles), async ({files, options, prettierOptions}) => runEslint(files, buildConfig(options, prettierOptions), {isQuiet: options.quiet})));

const files = await globFiles(patterns, mergeOptions(inputOptions));

const reports = await pMap(
files,
async filePath => {
const {options: foundOptions, prettierOptions} = mergeWithFileConfig({
...inputOptions,
filePath,
});
const options = buildConfig(foundOptions, prettierOptions);
return runEslint(filePath, options, {isQuiet: inputOptions.quiet});
},

This comment has been minimized.

Sign in to view

Copy link
@devinrhode2

devinrhode2 Oct 1, 2021

Contributor

@fisker - Responding to #599 (comment)

  • Maybe the multiple levels of async->await are causing issues here.

The memory issue I seem to have run into is quite troublesome, but the amount of code you've deleted suggests to me this change is definitely worth preserving...

At a high level, without really understanding this code, it seems we've somehow made this tradeoff:

  • Linting one file is slow
  • We've made that faster, but made linting all files slower

I am digging into the code some

This comment has been minimized.

Sign in to view

Copy link
@sindresorhus

sindresorhus Oct 1, 2021

Member

We could try limiting the concurrency and see if that help. pMap has a concurrency option.

This comment has been minimized.

Sign in to view

Copy link
@devinrhode2

devinrhode2 Oct 5, 2021

Contributor

I opened up my projects ./node_modules/xo folder, turns out in xo v0.44.0 does not have this pMap( call.
And, I think issue is not present in v0.45.0

I'll let you know if I find otherwise :)

This comment has been minimized.

Sign in to view

Copy link
@spence-s

spence-s Oct 8, 2021 •
edited
Loading

Contributor

All the activity here got me interested in this... At a glance, It looks like this commit went from grouping files that had the same config and running eslint once for each unique config to calculating the config AND running eslint for each individual file. This is definitely the regression if I am seeing it correctly. -- I ran a quick flame graph of xo with 0x and xo basically does nothing compared to eslint. -- Not 100% sure. I would not revert any commit, I would just try to add back in the grouping logic in a clean way and preserve the increased readability that these commits from @fisker offered.... I also may be way off here :) - this issue hasn't personally affected me.

Edit: it looks like @fisker already stated this is the reason for the change in the issues #599 (comment). So no new information here.

This comment has been minimized.

Sign in to view

Copy link
@devinrhode2

devinrhode2 Oct 11, 2021

Contributor

I wonder if using eslint 8 would bring any benefit, but it might be a couple months before that's enticing enough to move to (imo).

Conversely, maybe migrating to eslint 8 beta right now would be a really good idea, to identify potential issues, and get them fixes before official 1.0.0 is released

This comment has been minimized.

Sign in to view

Copy link
@fisker

fisker Oct 12, 2021

Author Contributor

I'vebeen very busy recently.

I think we should try concurrency option first.
And we can also try to cache the config search result. The files under config file dir should have the same result.
);

return mergeReports(reports.filter(Boolean));
};

const getFormatter = async name => {
Expand Down
72 changes: 3 additions & 69 deletions lib/options-manager.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,18 @@ import os from 'node:os';
import path from 'node:path';
import fsExtra from 'fs-extra';
import arrify from 'arrify';
import {mergeWith, groupBy, flow, pick} from 'lodash-es';
import {mergeWith, flow, pick} from 'lodash-es';
import pathExists from 'path-exists';
import findUp from 'find-up';
import findCacheDir from 'find-cache-dir';
import prettier from 'prettier';
import semver from 'semver';
import {cosmiconfig, cosmiconfigSync, defaultLoaders} from 'cosmiconfig';
import pReduce from 'p-reduce';
import {cosmiconfigSync, defaultLoaders} from 'cosmiconfig';
import micromatch from 'micromatch';
import JSON5 from 'json5';
import toAbsoluteGlob from 'to-absolute-glob';
import stringify from 'json-stable-stringify-without-jsonify';
import murmur from 'imurmurhash';
import isPathInside from 'is-path-inside';
import eslintrc from '@eslint/eslintrc';
import createEsmUtils from 'esm-utils';
import {
Expand All @@ -34,7 +32,7 @@ import {

const {__dirname, json, require} = createEsmUtils(import.meta);
const pkg = json.loadSync('../package.json');
const {outputJson, outputJsonSync} = fsExtra;
const {outputJsonSync} = fsExtra;
const {normalizePackageName} = eslintrc.Legacy.naming;
const resolveModule = eslintrc.Legacy.ModuleResolver.resolve;

Expand Down Expand Up @@ -139,69 +137,6 @@ const mergeWithFileConfig = options => {
return {options, prettierOptions};
};

/**
Find config for each files found by `lintFiles`.
The config files are searched starting from each files.
*/
const mergeWithFileConfigs = async (files, options, configFiles) => {
configFiles = configFiles.sort((a, b) => b.filepath.split(path.sep).length - a.filepath.split(path.sep).length);
const tsConfigs = {};

const groups = [...(await pReduce(files, async (configs, file) => {
const pkgConfigExplorer = cosmiconfig('engines', {searchPlaces: ['package.json'], stopDir: options.cwd});

const {config: xoOptions, filepath: xoConfigPath} = findApplicableConfig(file, configFiles) || {};
const {config: enginesOptions, filepath: enginesConfigPath} = await pkgConfigExplorer.search(file) || {};

let fileOptions = mergeOptions(options, xoOptions, enginesOptions);
fileOptions.cwd = xoConfigPath && path.dirname(xoConfigPath) !== fileOptions.cwd ? path.resolve(fileOptions.cwd, path.dirname(xoConfigPath)) : fileOptions.cwd;

const {hash, options: optionsWithOverrides} = applyOverrides(file, fileOptions);
fileOptions = optionsWithOverrides;

const prettierOptions = fileOptions.prettier ? await prettier.resolveConfig(file, {editorconfig: true}) || {} : {};

let tsConfigPath;
if (isTypescript(file)) {
let tsConfig;
const tsConfigExplorer = cosmiconfig([], {searchPlaces: ['tsconfig.json'], loaders: {'.json': (_, content) => JSON5.parse(content)}});
({config: tsConfig, filepath: tsConfigPath} = await tsConfigExplorer.search(file) || {});

fileOptions.tsConfigPath = tsConfigPath;
tsConfigs[tsConfigPath || ''] = tsConfig;
fileOptions.ts = true;
}

const cacheKey = stringify({xoConfigPath, enginesConfigPath, prettierOptions, hash, tsConfigPath: fileOptions.tsConfigPath, ts: fileOptions.ts});
const cachedGroup = configs.get(cacheKey);

configs.set(cacheKey, {
files: [file, ...(cachedGroup ? cachedGroup.files : [])],
options: cachedGroup ? cachedGroup.options : fileOptions,
prettierOptions,
});

return configs;
}, new Map())).values()];

await Promise.all(Object.entries(groupBy(groups.filter(({options}) => Boolean(options.ts)), group => group.options.tsConfigPath || '')).map(
([tsConfigPath, groups]) => {
const files = groups.flatMap(group => group.files);
const cachePath = getTsConfigCachePath(files, tsConfigPath, options.cwd);

for (const group of groups) {
group.options.tsConfigPath = cachePath;
}

return outputJson(cachePath, makeTSConfig(tsConfigs[tsConfigPath], tsConfigPath, files));
},
));

return groups;
};

const findApplicableConfig = (file, configFiles) => configFiles.find(({filepath}) => isPathInside(file, path.dirname(filepath)));

/**
Generate a unique and consistent path for the temporary `tsconfig.json`.
Hashing based on https://github.com/eslint/eslint/blob/cf38d0d939b62f3670cdd59f0143fd896fccd771/lib/cli-engine/lint-result-cache.js#L30
Expand Down Expand Up @@ -609,7 +544,6 @@ export {
mergeWithPrettierConfig,
normalizeOptions,
getIgnores,
mergeWithFileConfigs,
mergeWithFileConfig,
buildConfig,
applyOverrides,
Expand Down
2 changes: 0 additions & 2 deletions package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,9 +87,7 @@
"meow": "^10.1.1",
"micromatch": "^4.0.4",
"open-editor": "^3.0.0",
"p-filter": "^2.1.0",
"p-map": "^5.1.0",
"p-reduce": "^3.0.0",
"path-exists": "^4.0.0",
"prettier": "^2.3.2",
"semver": "^7.3.5",
Expand Down
153 changes: 0 additions & 153 deletions test/options-manager.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -586,159 +586,6 @@ test('mergeWithFileConfig: tsx files', async t => {
});
});

test('mergeWithFileConfigs: nested configs with prettier', async t => {
const cwd = path.resolve('fixtures', 'nested-configs');
const paths = [
'no-semicolon.js',
'child/semicolon.js',
'child-override/two-spaces.js',
'child-override/child-prettier-override/semicolon.js',
].map(file => path.resolve(cwd, file));
const result = await manager.mergeWithFileConfigs(paths, {cwd}, [
{
filepath: path.resolve(cwd, 'child-override', 'child-prettier-override', 'package.json'),
config: {overrides: [{files: 'semicolon.js', prettier: true}]},
},
{filepath: path.resolve(cwd, 'package.json'), config: {semicolon: true}},
{
filepath: path.resolve(cwd, 'child-override', 'package.json'),
config: {overrides: [{files: 'two-spaces.js', space: 4}]},
},
{filepath: path.resolve(cwd, 'child', 'package.json'), config: {semicolon: false}},
]);

t.deepEqual(result, [
{
files: [path.resolve(cwd, 'no-semicolon.js')],
options: {
semicolon: true,
cwd,
extensions: DEFAULT_EXTENSION,
ignores: DEFAULT_IGNORES,
},
prettierOptions: {},
},
{
files: [path.resolve(cwd, 'child/semicolon.js')],
options: {
semicolon: false,
cwd: path.resolve(cwd, 'child'),
extensions: DEFAULT_EXTENSION,
ignores: DEFAULT_IGNORES,
},
prettierOptions: {},
},
{
files: [path.resolve(cwd, 'child-override/two-spaces.js')],
options: {
space: 4,
rules: {},
settings: {},
globals: [],
envs: [],
plugins: [],
extends: [],
cwd: path.resolve(cwd, 'child-override'),
extensions: DEFAULT_EXTENSION,
ignores: DEFAULT_IGNORES,
},
prettierOptions: {},
},
{
files: [path.resolve(cwd, 'child-override/child-prettier-override/semicolon.js')],
options: {
prettier: true,
rules: {},
settings: {},
globals: [],
envs: [],
plugins: [],
extends: [],
cwd: path.resolve(cwd, 'child-override', 'child-prettier-override'),
extensions: DEFAULT_EXTENSION,
ignores: DEFAULT_IGNORES,
},
prettierOptions: {endOfLine: 'lf', semi: false, useTabs: true},
},
]);
});

test('mergeWithFileConfigs: typescript files', async t => {
const cwd = path.resolve('fixtures', 'typescript');
const paths = ['two-spaces.tsx', 'child/extra-semicolon.ts', 'child/sub-child/four-spaces.ts'].map(file => path.resolve(cwd, file));
const configFiles = [
{filepath: path.resolve(cwd, 'child/sub-child/package.json'), config: {space: 2}},
{filepath: path.resolve(cwd, 'package.json'), config: {space: 4}},
{filepath: path.resolve(cwd, 'child/package.json'), config: {semicolon: false}},
];
const result = await manager.mergeWithFileConfigs(paths, {cwd}, configFiles);

t.deepEqual(omit(result[0], 'options.tsConfigPath'), {
files: [path.resolve(cwd, 'two-spaces.tsx')],
options: {
space: 4,
cwd,
extensions: DEFAULT_EXTENSION,
ignores: DEFAULT_IGNORES,
ts: true,
},
prettierOptions: {},
});
t.deepEqual(await readJson(result[0].options.tsConfigPath), {
files: [path.resolve(cwd, 'two-spaces.tsx')],
compilerOptions: {
newLine: 'lf',
noFallthroughCasesInSwitch: true,
noImplicitReturns: true,
noUnusedLocals: true,
noUnusedParameters: true,
strict: true,
target: 'es2018',
},
});

t.deepEqual(omit(result[1], 'options.tsConfigPath'), {
files: [path.resolve(cwd, 'child/extra-semicolon.ts')],
options: {
semicolon: false,
cwd: path.resolve(cwd, 'child'),
extensions: DEFAULT_EXTENSION,
ignores: DEFAULT_IGNORES,
ts: true,
},
prettierOptions: {},
});

t.deepEqual(omit(result[2], 'options.tsConfigPath'), {
files: [path.resolve(cwd, 'child/sub-child/four-spaces.ts')],
options: {
space: 2,
cwd: path.resolve(cwd, 'child/sub-child'),
extensions: DEFAULT_EXTENSION,
ignores: DEFAULT_IGNORES,
ts: true,
},
prettierOptions: {},
});

// Verify that we use the same temporary tsconfig.json for both files group sharing the same original tsconfig.json even if they have different xo config
t.is(result[1].options.tsConfigPath, result[2].options.tsConfigPath);
t.deepEqual(await readJson(result[1].options.tsConfigPath), {
extends: path.resolve(cwd, 'child/tsconfig.json'),
files: [path.resolve(cwd, 'child/extra-semicolon.ts'), path.resolve(cwd, 'child/sub-child/four-spaces.ts')],
include: [
slash(path.resolve(cwd, 'child/**/*.ts')),
slash(path.resolve(cwd, 'child/**/*.tsx')),
],
});

const secondResult = await manager.mergeWithFileConfigs(paths, {cwd}, configFiles);

// Verify that on each run the options.tsConfigPath is consistent to preserve ESLint cache
t.is(result[0].options.tsConfigPath, secondResult[0].options.tsConfigPath);
t.is(result[1].options.tsConfigPath, secondResult[1].options.tsConfigPath);
});

test('applyOverrides', t => {
t.deepEqual(
manager.applyOverrides(
Expand Down

0 comments on commit e2e715d

Please sign in to comment.