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 #43, #44, #45 #47

Merged
merged 5 commits into from
Nov 30, 2020
Merged

Fix #43, #44, #45 #47

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
db4f6d3
feat: add failing test
jsg2021 Nov 20, 2020
9c48a8f
fix: #44 keep all results across runs
jsg2021 Nov 20, 2020
9883b78
fix: #45 - include eslint options in types
jsg2021 Nov 20, 2020
85df345
fix: 43
jsg2021 Nov 30, 2020
034a29a
Merge branch 'master' into fix-44
ricardogobbosouza Nov 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 2 additions & 17 deletions declarations/getESLint.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,23 +23,8 @@ export function loadESLintThreaded(poolSize: number, options: Options): Linter;
export default function getESLint({ threads, ...options }: Options): Linter;
export type ESLint = import('eslint').ESLint;
export type LintResult = import('eslint').ESLint.LintResult;
export type Options = {
context?: string | undefined;
emitError?: boolean | undefined;
emitWarning?: boolean | undefined;
eslintPath?: string | undefined;
exclude?: string | string[] | undefined;
extensions?: string | string[] | undefined;
failOnError?: boolean | undefined;
failOnWarning?: boolean | undefined;
files?: string | string[] | undefined;
fix?: boolean | undefined;
formatter?: string | import('./options').FormatterFunction | undefined;
lintDirtyModulesOnly?: boolean | undefined;
quiet?: boolean | undefined;
outputReport?: import('./options').OutputReport | undefined;
threads?: number | boolean | undefined;
};
export type Options = import('./options').PluginOptions &
import('eslint').ESLint.Options;
export type AsyncTask = () => Promise<void>;
export type LintTask = (files: string | string[]) => Promise<LintResult[]>;
export type Worker = JestWorker & {
Expand Down
21 changes: 3 additions & 18 deletions declarations/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ export class ESLintWebpackPlugin {
* @param {Options} options
*/
constructor(options?: Options);
options: import('./options').Options;
options: import('./options').PluginOptions;
/**
* @param {Compiler} compiler
*/
Expand All @@ -22,20 +22,5 @@ export class ESLintWebpackPlugin {
}
export default ESLintWebpackPlugin;
export type Compiler = import('webpack').Compiler;
export type Options = {
context?: string | undefined;
emitError?: boolean | undefined;
emitWarning?: boolean | undefined;
eslintPath?: string | undefined;
exclude?: string | string[] | undefined;
extensions?: string | string[] | undefined;
failOnError?: boolean | undefined;
failOnWarning?: boolean | undefined;
files?: string | string[] | undefined;
fix?: boolean | undefined;
formatter?: string | import('./options').FormatterFunction | undefined;
lintDirtyModulesOnly?: boolean | undefined;
quiet?: boolean | undefined;
outputReport?: import('./options').OutputReport | undefined;
threads?: number | boolean | undefined;
};
export type Options = import('./options').PluginOptions &
import('eslint').ESLint.Options;
34 changes: 5 additions & 29 deletions declarations/linter.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,3 @@
/** @typedef {import('eslint').ESLint} ESLint */
/** @typedef {import('eslint').ESLint.Formatter} Formatter */
/** @typedef {import('eslint').ESLint.LintResult} LintResult */
/** @typedef {import('webpack').Compiler} Compiler */
/** @typedef {import('webpack').Compilation} Compilation */
/** @typedef {import('webpack-sources').Source} Source */
/** @typedef {import('./options').Options} Options */
/** @typedef {import('./options').FormatterFunction} FormatterFunction */
/** @typedef {(compilation: Compilation) => Promise<void>} GenerateReport */
/** @typedef {{errors?: ESLintError, warnings?: ESLintError, generateReportAsset?: GenerateReport}} Report */
/** @typedef {() => Promise<Report>} Reporter */
/** @typedef {(files: string|string[]) => void} Linter */
/**
* @param {Options} options
* @param {Compilation} compilation
Expand All @@ -28,23 +16,8 @@ export type LintResult = import('eslint').ESLint.LintResult;
export type Compiler = import('webpack').Compiler;
export type Compilation = import('webpack').Compilation;
export type Source = import('webpack-sources/lib/Source');
export type Options = {
context?: string | undefined;
emitError?: boolean | undefined;
emitWarning?: boolean | undefined;
eslintPath?: string | undefined;
exclude?: string | string[] | undefined;
extensions?: string | string[] | undefined;
failOnError?: boolean | undefined;
failOnWarning?: boolean | undefined;
files?: string | string[] | undefined;
fix?: boolean | undefined;
formatter?: string | import('./options').FormatterFunction | undefined;
lintDirtyModulesOnly?: boolean | undefined;
quiet?: boolean | undefined;
outputReport?: import('./options').OutputReport | undefined;
threads?: number | boolean | undefined;
};
export type Options = import('./options').PluginOptions &
import('eslint').ESLint.Options;
export type FormatterFunction = (
results: import('eslint').ESLint.LintResult[],
data?: import('eslint').ESLint.LintResultData | undefined
Expand All @@ -59,4 +32,7 @@ export type Report = {
};
export type Reporter = () => Promise<Report>;
export type Linter = (files: string | string[]) => void;
export type LintResultMap = {
[files: string]: import('eslint').ESLint.LintResult;
};
import ESLintError from './ESLintError';
10 changes: 6 additions & 4 deletions declarations/options.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
* @property {string|FormatterFunction=} formatter
*/
/**
* @typedef {Object} Options
* @typedef {Object} PluginOptions
* @property {string=} context
* @property {boolean=} emitError
* @property {boolean=} emitWarning
Expand All @@ -30,11 +30,12 @@
* @property {OutputReport=} outputReport
* @property {number|boolean=} threads
*/
/** @typedef {PluginOptions & ESLintOptions} Options */
/**
* @param {Options} pluginOptions
* @returns {Options}
* @returns {PluginOptions}
*/
export function getOptions(pluginOptions: Options): Options;
export function getOptions(pluginOptions: Options): PluginOptions;
/**
* @param {Options} loaderOptions
* @returns {ESLintOptions}
Expand All @@ -51,7 +52,7 @@ export type OutputReport = {
filePath?: string | undefined;
formatter?: (string | FormatterFunction) | undefined;
};
export type Options = {
export type PluginOptions = {
context?: string | undefined;
emitError?: boolean | undefined;
emitWarning?: boolean | undefined;
Expand All @@ -68,3 +69,4 @@ export type Options = {
outputReport?: OutputReport | undefined;
threads?: (number | boolean) | undefined;
};
export type Options = PluginOptions & import('eslint').ESLint.Options;
10 changes: 8 additions & 2 deletions src/getESLint.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,19 @@ export function loadESLint(options) {

// Filter out loader options before passing the options to ESLint.
const eslint = new ESLint(getESLintOptions(options));
const lintFiles = eslint.lintFiles.bind(eslint);

return {
threads: 1,
ESLint,
eslint,
lintFiles,
lintFiles: async (files) => {
const results = await eslint.lintFiles(files);
// istanbul ignore else
if (options.fix) {
await ESLint.outputFixes(results);
}
return results;
},
// no-op for non-threaded
cleanup: async () => {},
};
Expand Down
45 changes: 36 additions & 9 deletions src/linter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,17 @@ import getESLint from './getESLint';
/** @typedef {{errors?: ESLintError, warnings?: ESLintError, generateReportAsset?: GenerateReport}} Report */
/** @typedef {() => Promise<Report>} Reporter */
/** @typedef {(files: string|string[]) => void} Linter */
/** @typedef {{[files: string]: LintResult}} LintResultMap */

/** @type {WeakMap<Compiler, LintResultMap>} */
const resultStorage = new WeakMap();

/**
* @param {Options} options
* @param {Compilation} compilation
* @returns {{lint: Linter, report: Reporter}}
*/
export default function linter(options, compilation) {
/** @type {ESLint} */
let ESLint;

/** @type {ESLint} */
let eslint;

Expand All @@ -36,8 +38,10 @@ export default function linter(options, compilation) {
/** @type {Promise<LintResult[]>[]} */
const rawResults = [];

const crossRunResultStorage = getResultStorage(compilation);

try {
({ ESLint, eslint, lintFiles, cleanup } = getESLint(options));
({ eslint, lintFiles, cleanup } = getESLint(options));
} catch (e) {
throw new ESLintError(e.message);
}
Expand All @@ -51,6 +55,9 @@ export default function linter(options, compilation) {
* @param {string | string[]} files
*/
function lint(files) {
for (const file of asList(files)) {
delete crossRunResultStorage[file];
}
Copy link
Member

Choose a reason for hiding this comment

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

Why we need manually delete it? When you remove something from WeakMap often means that you are using it out of place

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thats not the WeakMap. The WeakMap keeps a compiler instance to (file=>lintresult map) mapping.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we lint a file and it does not contain errors, we don't want the old result just in case we do not get a result back from the linter.

Copy link
Member

Choose a reason for hiding this comment

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

Why we need to keep old result?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's the point of #44 ... we need to keep reporting results until they are resolved. In watch mode if the user edits a file we don't want to discard all the other file results and only show the one file's results.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In each run of the compiler, we only get the results of the files we lint... in watch mode, that may only be one file. But this should be cumulative not just the last run.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for explanation

rawResults.push(
lintFiles(files).catch((e) => {
compilation.errors.push(e);
Expand All @@ -61,7 +68,7 @@ export default function linter(options, compilation) {

async function report() {
// Filter out ignored files.
const results = await removeIgnoredWarnings(
let results = await removeIgnoredWarnings(
eslint,
// Get the current results, resetting the rawResults to empty
await flatten(rawResults.splice(0, rawResults.length))
Expand All @@ -74,12 +81,12 @@ export default function linter(options, compilation) {
return {};
}

// if enabled, use eslint autofixing where possible
if (options.fix) {
// @ts-ignore
await ESLint.outputFixes(results);
for (const result of results) {
crossRunResultStorage[result.filePath] = result;
}

results = Object.values(crossRunResultStorage);

const formatter = await loadFormatter(eslint, options.formatter);
const { errors, warnings } = formatResults(
formatter,
Expand Down Expand Up @@ -276,3 +283,23 @@ async function flatten(results) {
const flat = (acc, list) => [...acc, ...list];
return (await Promise.all(results)).reduce(flat, []);
}

/**
* @param {Compilation} compilation
* @returns {LintResultMap}
*/
function getResultStorage({ compiler }) {
let storage = resultStorage.get(compiler);
if (!storage) {
resultStorage.set(compiler, (storage = {}));
}
return storage;
}

/**
* @param {string | string[]} x
*/
function asList(x) {
/* istanbul ignore next */
return Array.isArray(x) ? x : [x];
}
6 changes: 4 additions & 2 deletions src/options.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import schema from './options.json';
*/

/**
* @typedef {Object} Options
* @typedef {Object} PluginOptions
* @property {string=} context
* @property {boolean=} emitError
* @property {boolean=} emitWarning
Expand All @@ -38,9 +38,11 @@ import schema from './options.json';
* @property {number|boolean=} threads
*/

/** @typedef {PluginOptions & ESLintOptions} Options */

/**
* @param {Options} pluginOptions
* @returns {Options}
* @returns {PluginOptions}
*/
export function getOptions(pluginOptions) {
const options = {
Expand Down
18 changes: 15 additions & 3 deletions src/worker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,24 +6,36 @@ Object.assign(module.exports, {
setup,
});

/** @type {{ new (arg0: import("eslint").ESLint.Options): import("eslint").ESLint; outputFixes: (arg0: import("eslint").ESLint.LintResult[]) => any; }} */
let ESLint;

/** @type {ESLint} */
let eslint;

/** @type {boolean} */
let fix;

/**
* @typedef {object} setupOptions
* @property {string=} eslintPath - import path of eslint
* @property {ESLintOptions=} eslintOptions - linter options
*
* @param {setupOptions} arg0 - setup worker
*/
function setup({ eslintPath, eslintOptions }) {
const { ESLint } = require(eslintPath || 'eslint');
function setup({ eslintPath, eslintOptions = {} }) {
fix = !!(eslintOptions && eslintOptions.fix);
({ ESLint } = require(eslintPath || 'eslint'));
eslint = new ESLint(eslintOptions);
}

/**
* @param {string | string[]} files
*/
async function lintFiles(files) {
return eslint.lintFiles(files);
const result = await eslint.lintFiles(files);
// if enabled, use eslint autofixing where possible
if (fix) {
await ESLint.outputFixes(result);
}
return result;
}
46 changes: 29 additions & 17 deletions test/autofix.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { join } from 'path';

import { copySync, removeSync } from 'fs-extra';
import { copySync, removeSync, readFileSync } from 'fs-extra';

import pack from './utils/pack';

Expand All @@ -15,20 +15,32 @@ describe('autofix stop', () => {
removeSync(entry);
});

it('should not throw error if file ok after auto-fixing', (done) => {
const compiler = pack('fixable-clone', {
fix: true,
extensions: ['js', 'cjs', 'mjs'],
overrideConfig: {
rules: { semi: ['error', 'always'] },
},
});

compiler.run((err, stats) => {
expect(err).toBeNull();
expect(stats.hasWarnings()).toBe(false);
expect(stats.hasErrors()).toBe(false);
done();
});
});
test.each([[{}], [{ threads: false }]])(
'should not throw error if file ok after auto-fixing',
(cfg, done) => {
const compiler = pack('fixable-clone', {
...cfg,
fix: true,
extensions: ['js', 'cjs', 'mjs'],
overrideConfig: {
rules: { semi: ['error', 'always'] },
},
});

compiler.run((err, stats) => {
expect(err).toBeNull();
expect(stats.hasWarnings()).toBe(false);
expect(stats.hasErrors()).toBe(false);
expect(readFileSync(entry).toString('utf8')).toMatchInlineSnapshot(`
"function foo() {
return true;
}
foo();
"
`);
done();
});
}
);
});
Loading