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.

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

feat: use ESLint instead of CLIEngine when available #128

Merged
merged 1 commit into from
Sep 22, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions .eslintrc.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
{
"extends": ["airbnb-base", "prettier"],
"plugins": ["prettier", "jest"],
"parserOptions": {
"ecmaVersion": 11
Copy link
Member Author

Choose a reason for hiding this comment

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

needed for optional chaining (babel transpiles it away)

},
"rules": {
"no-underscore-dangle": 0,
"arrow-body-style": 0,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
const { ESLint } = require('eslint');

module.exports = {
cliOptions: {
global: ['hello'],
// `ESLint` requires this to be an object, not an array
global: ESLint ? { hello: true } : ['hello'],
Copy link
Member Author

@SimenB SimenB Sep 17, 2021

Choose a reason for hiding this comment

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

this is main reason I think this is a breaking change. While it's easy enough to normalize this specific field, there's probably others we'd miss - cleaner to just make a breaking change

Copy link
Member Author

Choose a reason for hiding this comment

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

thinking about it, #125 might be considered breaking as well, if people previously ignored an engine warning and used this package on node 6. I don't consider it a breaking change, but bundling these together in a new major removes any ambiguity 🤷

Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems pretty valuable to try to support both eslint 8 and < 8 at the same time

Copy link
Member Author

@SimenB SimenB Sep 17, 2021

Choose a reason for hiding this comment

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

sure, but I don't think it's our responsibility to coerce user input into the correct type. Consumers should configure the the version of ESLint they choose to use correctly.

(I know we do some coercion already, so I'm also fine with special casing globals so our tests are happy. I'd still consider this a breaking change since it's a moving target though)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh true, fair point - i think it’s 100% fine if a user switches to eslint 8, and this runner breaks until they also fix their configuration (altho hopefully they get a very clear error message)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, the error message from eslint is pretty good 👍 Specifically in this case if I leave it as an array

ESLint configuration in CLIOptions is invalid:
      - Property "globals" is the wrong type (expected object but got `["hello"]`).

},
};
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@
"dependencies": {
"chalk": "^3.0.0",
"cosmiconfig": "^6.0.0",
"create-jest-runner": "^0.6.0"
"create-jest-runner": "^0.6.0",
"dot-prop": "^5.3.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This author is planning to make all their packages ESM-only - I’d strongly suggest avoiding adding them as a dependency.

Copy link
Member Author

Choose a reason for hiding this comment

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

I went for v5 since v6 doesn't support node 8. We can migrate to some other module later if needed. Unless you have a good suggestion for a module that's not this or lodash? 😀

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need a module for this? It always seems hacky to me to rely on converting a dotted string to object navigation; maybe there’s a different approach?

Copy link
Member Author

Choose a reason for hiding this comment

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

not sure - the ESLint class requires a bunch of stuff in configOverrides instead of in the root object, and deep merging is quite hellish in JS (without some module). Going with a dotted setter seemed like a nice shortcut for the specific CLI options we've added support for

(I'm not entirely sure why we have this normalization instead of passing through raw as-is, but that's a separate discussion)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll land this, and if we figure out some cleaner way of dealing with this we can revisit. It's not exposed, so it's an implementation detail we can change at will

},
"devDependencies": {
"@babel/cli": "^7.10.4",
Expand Down
21 changes: 13 additions & 8 deletions src/runner/__tests__/runESLint.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,12 @@ const runESLintRunnerWithMockedEngine = options => {

executeOnFiles() {
return {
results:
options.cliEngine.errorCount > 0
? [{ errorCount: options.cliEngine.errorCount, warningCount: 0 }]
: [],
errorCount: options.cliEngine.errorCount,
warningCount: 0,
};
}

Expand All @@ -25,7 +30,7 @@ const runESLintRunnerWithMockedEngine = options => {
return runESLint({ extraOptions: {}, ...options.runESLint });
};

it('Requires the config setupTestFrameworkScriptFile when specified', () => {
it('Requires the config setupTestFrameworkScriptFile when specified', async () => {
const setupFile = path.join(__dirname, './path/to/setupFile.js');

let setupFileWasLoaded = false;
Expand All @@ -37,7 +42,7 @@ it('Requires the config setupTestFrameworkScriptFile when specified', () => {
{ virtual: true },
);

runESLintRunnerWithMockedEngine({
await runESLintRunnerWithMockedEngine({
cliEngine: {
ignoredFiles: ['/path/to/file.test.js'],
errorCount: 0,
Expand All @@ -53,8 +58,8 @@ it('Requires the config setupTestFrameworkScriptFile when specified', () => {
expect(setupFileWasLoaded).toBeTruthy();
});

it('Returns "skipped" when the test path is ignored', () => {
const result = runESLintRunnerWithMockedEngine({
it('Returns "skipped" when the test path is ignored', async () => {
const result = await runESLintRunnerWithMockedEngine({
cliEngine: {
ignoredFiles: ['/path/to/file.test.js'],
errorCount: 0,
Expand All @@ -73,8 +78,8 @@ it('Returns "skipped" when the test path is ignored', () => {
});
});

it('Returns "passed" when the test passed', () => {
const result = runESLintRunnerWithMockedEngine({
it('Returns "passed" when the test passed', async () => {
const result = await runESLintRunnerWithMockedEngine({
cliEngine: {
ignoredFiles: [],
errorCount: 0,
Expand All @@ -92,8 +97,8 @@ it('Returns "passed" when the test passed', () => {
});
});

it('Returns "fail" when the test failed', () => {
const result = runESLintRunnerWithMockedEngine({
it('Returns "fail" when the test failed', async () => {
const result = await runESLintRunnerWithMockedEngine({
cliEngine: {
ignoredFiles: [],
errorCount: 1,
Expand Down
70 changes: 53 additions & 17 deletions src/runner/runESLint.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
const { pass, fail, skip } = require('create-jest-runner');
const { CLIEngine } = require('eslint');
const { CLIEngine, ESLint } = require('eslint');
const getESLintOptions = require('../utils/getESLintOptions');

const getComputedFixValue = ({ fix, quiet, fixDryRun }) => {
Expand All @@ -9,54 +9,89 @@ const getComputedFixValue = ({ fix, quiet, fixDryRun }) => {
return undefined;
};

const ESLintEngine = ESLint || CLIEngine;

let cachedValues;
const getCachedValues = (config, extraOptions) => {
if (!cachedValues) {
const { cliOptions: baseCliOptions } = getESLintOptions(config);
const useEngine = ESLint == null;
const { cliOptions: baseCliOptions } = getESLintOptions(config, !useEngine);
const cliOptions = {
...baseCliOptions,
fix: getComputedFixValue(baseCliOptions),
...extraOptions,
};
const cli = new CLIEngine(cliOptions);
const formatter = cli.getFormatter(cliOptions.format);

cachedValues = { cli, formatter, cliOptions };
// these are not constructor args, so remove them
const { fixDryRun, format, maxWarnings, quiet } = cliOptions;
delete cliOptions.fixDryRun;
delete cliOptions.format;
delete cliOptions.maxWarnings;
delete cliOptions.quiet;

const cli = useEngine ? new CLIEngine(cliOptions) : new ESLint(cliOptions);

cachedValues = {
isPathIgnored: cli.isPathIgnored.bind(cli),
lintFiles: (...args) => {
if (useEngine) {
return cli.executeOnFiles(...args).results;
}

return cli.lintFiles(...args);
},
formatter: async (...args) => {
if (useEngine) {
return cli.getFormatter(format)(...args);
}

const formatter = await cli.loadFormatter(format);

return formatter.format(...args);
},
cliOptions: {
...cliOptions,
fixDryRun,
maxWarnings,
quiet,
},
};
}

return cachedValues;
};

const runESLint = ({ testPath, config, extraOptions }) => {
const runESLint = async ({ testPath, config, extraOptions }) => {
const start = Date.now();

if (config.setupTestFrameworkScriptFile) {
// eslint-disable-next-line import/no-dynamic-require,global-require
require(config.setupTestFrameworkScriptFile);
}

const { cli, formatter, cliOptions } = getCachedValues(config, extraOptions);
const { isPathIgnored, lintFiles, formatter, cliOptions } = getCachedValues(
config,
extraOptions,
);

if (cli.isPathIgnored(testPath)) {
if (await isPathIgnored(testPath)) {
const end = Date.now();
return skip({ start, end, test: { path: testPath, title: 'ESLint' } });
}

const report = cli.executeOnFiles([testPath]);
const report = await lintFiles([testPath]);

if (cliOptions.fix && !cliOptions.fixDryRun) {
CLIEngine.outputFixes(report);
await ESLintEngine.outputFixes(report);
}

const end = Date.now();

const message = formatter(
cliOptions.quiet
? CLIEngine.getErrorResults(report.results)
: report.results,
const message = await formatter(
cliOptions.quiet ? ESLintEngine.getErrorResults(report) : report,
);

if (report.errorCount > 0) {
if (report[0]?.errorCount > 0) {
return fail({
start,
end,
Expand All @@ -65,7 +100,8 @@ const runESLint = ({ testPath, config, extraOptions }) => {
}

const tooManyWarnings =
cliOptions.maxWarnings >= 0 && report.warningCount > cliOptions.maxWarnings;
cliOptions.maxWarnings >= 0 &&
report[0]?.warningCount > cliOptions.maxWarnings;
if (tooManyWarnings) {
return fail({
start,
Expand All @@ -84,7 +120,7 @@ const runESLint = ({ testPath, config, extraOptions }) => {
test: { path: testPath, title: 'ESLint' },
});

if (!cliOptions.quiet && report.warningCount > 0) {
if (!cliOptions.quiet && report[0]?.warningCount > 0) {
result.console = [{ message, origin: '', type: 'warn' }];
}

Expand Down
2 changes: 1 addition & 1 deletion src/utils/__tests__/normalizeConfig.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ const normalizeConfig = require('../normalizeConfig');
const normalizeCLIOptions = cliOptions =>
normalizeConfig({ cliOptions }).cliOptions;

it('ignores unkown options', () => {
it('ignores unknown options', () => {
expect(normalizeCLIOptions({ other: true })).not.toMatchObject({
other: true,
});
Expand Down
6 changes: 3 additions & 3 deletions src/utils/getESLintOptions.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,14 @@ const normalizeConfig = require('./normalizeConfig');

const explorer = cosmiconfigSync('jest-runner-eslint');

const getESLintOptions = config => {
const getESLintOptions = (config, newApi) => {
const result = explorer.search(config.rootDir);

if (result) {
return normalizeConfig(result.config);
return normalizeConfig(result.config, newApi);
}

return normalizeConfig({});
return normalizeConfig({}, newApi);
};

module.exports = getESLintOptions;
66 changes: 55 additions & 11 deletions src/utils/normalizeConfig.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
const dotProp = require('dot-prop');

const identity = v => v;
const negate = v => !v;
const asArray = v => (typeof v === 'string' ? [v] : v);
Expand All @@ -12,7 +14,7 @@ const asInt = v => {
return int;
};

const BASE_CONFIG = {
const OLD_BASE_CONFIG = {
cache: {
default: false,
},
Expand Down Expand Up @@ -40,7 +42,7 @@ const BASE_CONFIG = {
default: false,
},
format: {
default: null,
default: undefined,
},
global: {
name: 'globals',
Expand Down Expand Up @@ -103,28 +105,70 @@ const BASE_CONFIG = {
},
};

const BASE_CONFIG = {
...OLD_BASE_CONFIG,
config: {
name: 'overrideConfigFile',
default: null,
},
env: {
name: 'overrideConfig.env',
default: {},
},
global: {
name: 'overrideConfig.globals',
default: {},
},
ignorePattern: {
name: 'overrideConfig.ignorePatterns',
default: [],
transform: asArray,
},
parser: {
name: 'overrideConfig.parser',
default: null,
},
parserOptions: {
name: 'overrideConfig.parserOptions',
default: {},
},
plugin: {
name: 'overrideConfig.plugins',
default: [],
transform: asArray,
},
reportUnusedDisableDirectives: {
default: null,
},
rules: {
name: 'overrideConfig.rules',
default: {},
},
};

/* eslint-disable no-param-reassign */
const normalizeCliOptions = rawConfig =>
Object.keys(BASE_CONFIG).reduce((config, key) => {
const normalizeCliOptions = (rawConfig, newApi) => {
const configToUse = newApi ? BASE_CONFIG : OLD_BASE_CONFIG;
return Object.keys(configToUse).reduce((config, key) => {
const {
name = key,
transform = identity,
default: defaultValue,
} = BASE_CONFIG[key];
} = configToUse[key];

const value = rawConfig[key] !== undefined ? rawConfig[key] : defaultValue;

return {
...config,
[name]: transform(value),
};
dotProp.set(config, name, transform(value));

return config;
}, {});
};
/* eslint-enable no-param-reassign */

const normalizeConfig = config => {
const normalizeConfig = (config, newApi) => {
return {
...config,
cliOptions: normalizeCliOptions(config.cliOptions || {}),
cliOptions: normalizeCliOptions(config.cliOptions || {}, newApi),
};
};

Expand Down
12 changes: 12 additions & 0 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -2036,6 +2036,13 @@ domexception@^2.0.1:
dependencies:
webidl-conversions "^5.0.0"

dot-prop@^5.3.0:
version "5.3.0"
resolved "https://registry.yarnpkg.com/dot-prop/-/dot-prop-5.3.0.tgz#90ccce708cd9cd82cc4dc8c3ddd9abdd55b20e88"
integrity sha512-QM8q3zDe58hqUqjraQOmzZ1LIH9SWQJTlEKCH4kJ2oQvLZk7RbQXvtDM2XEq3fwkV9CCvvH4LA0AV+ogFsBM2Q==
dependencies:
is-obj "^2.0.0"

electron-to-chromium@^1.3.830:
version "1.3.842"
resolved "https://registry.yarnpkg.com/electron-to-chromium/-/electron-to-chromium-1.3.842.tgz#641e414012dded277468892c0156cb01984f4f6f"
Expand Down Expand Up @@ -2956,6 +2963,11 @@ is-number@^7.0.0:
resolved "https://registry.yarnpkg.com/is-number/-/is-number-7.0.0.tgz#7535345b896734d5f80c4d06c50955527a14f12b"
integrity sha512-41Cifkg6e8TylSpdtTpeLVMqvSBEVzTttHvERD741+pnZ8ANv0004MRL43QKPDlK9cGvNp6NZWZUBlbGXYxxng==

is-obj@^2.0.0:
version "2.0.0"
resolved "https://registry.yarnpkg.com/is-obj/-/is-obj-2.0.0.tgz#473fb05d973705e3fd9620545018ca8e22ef4982"
integrity sha512-drqDG3cbczxxEJRoOXcOjtdp1J/lyp1mNn0xaznRs8+muBhgQcrnbspox5X5fOw0HnMnbfDzvnEMEtqDEJEo8w==

is-plain-object@^2.0.3, is-plain-object@^2.0.4:
version "2.0.4"
resolved "https://registry.yarnpkg.com/is-plain-object/-/is-plain-object-2.0.4.tgz#2c163b3fafb1b606d9d17928f05c2a1c38e07677"
Expand Down