feat: react-scripts lint command #1217

[maciej-ka]

Adds lint command to react-scripts.

Typical use will be to:

1. add to package.json scripts:

 "scripts": {
...,
"lint": "react-scripts lint"
}

EDIT: PR provides a default configuration for ESlinter, so these are no longer needed:
2. yarn lint --init
3. yarn to install new dependencies
4. yarn lint to use

Script passes all options to eslint, so this will work yarn lint --fix. Files checked are: .js, .jsx and .mjs in src folder. This includes test files too.

[gaearon]

This doesn’t work after eject.

[maciej-ka]

That's on purpose to drive away from ejecting.
Thank you for checking. I'll try to solve this problem today.

[maciej-ka]

Fixed and tested. Should work after ejecting now.

[gaearon]

Why does it need a separate case, and not using the same logic as above?

[maciej-ka]

I wasn't sure is node spawn needed in this case. But now I see. This way script feels no difference between being called with react-script command and after eject with node scripts/command.

[gaearon]

What is this for? I don't understand.

[maciej-ka]

A part of eslint --init wizard is to install dependencies using npm i. Since we have yarn, this will make a small mess. This line of code is to inform, that after react-scripts lint project has to be rebuilt with rm node_modules && yarn and possibly rm package-lock.json.

[maciej-ka]

I added this info after losing some time figuring out why all commands don't work anymore after react-scripts lint.

[maciej-ka]

Lint case unified with rest of commands in bin/react-scripts.js.
Small instruction after running lint --init wizard stayed. Maybe let's remove it after eslint --init gets new option --use-yarn?

[maciej-ka]

@gaearon should project that was built by create-react-app come with default eslinter configuration? This is a suggested approach by ESLinter team.

At the moment we don't provide such a file and users will probably need to make a copy of rules from other project or create a new eslinter config with react-scripts lint --init wizard.

[gaearon]

I don’t think we want to add any wizard here.

By default it should use the lint config that’s already used by default by commands like npm start. Specifically, eslint-config-react-app.

[maciej-ka]

Thanks. I'll look for a way to integrate them into react-script lint.

[maciej-ka]

Added a default ESlint configuration, removed warning about --init.

[gaearon]

See below; we'll also need integration tests for this.

For example you could use some existing integration test in the tasks/ folder and verify that running ./node_modules/.bin/react-scripts lint before ejecting and node scripts/lint.js after ejecting doesn't break.

[gaearon]

After ejecting we already put eslint config into the package.json (as far as I remember).
Why is this necessary?

[maciej-ka]

Great to know ... now ... after few days of working on it (madman laugh: iiihahahaha).

[maciej-ka]

This file was added to run linter before ejecting.
Do you know how is it possible to have same effect but without such a file?

Side note: ESlinter can be run programmicaly:

constCLIEngine=require("eslint").CLIEngine;constcli=newCLIEngine({configFile: require.resolve('../config/eslint.json'),fix: process.argv.slice(2).indexOf('--fix')>=0});constreport=cli.executeOnFiles(["src/**/*.{js,jsx,mjs}"]);CLIEngine.outputFixes(report);constformatter=cli.getFormatter();console.log(formatter(report.results));

But it still seems it needs a config file. In supported options it is possible to pass rules, but not extends. And, sadly, running above snippet after ejecting doesn't work, because eslint cannot find jsx-a11y definitions, even with this line added: plugins: ['import', 'flowtype', 'jsx-a11y', 'react']

[gaearon]

Can baseConfig argument help?

[maciej-ka]

Yes, it works
baseConfig: { extends: "react-app" },

however, after ejecting linter now reports

/private/tmp/demo-15/src/App.js
1:1 warning Definition for rule 'jsx-a11y/href-no-hash' was not found jsx-a11y/href-no-hash
/private/tmp/demo-15/src/App.test.js
1:1 warning Definition for rule 'jsx-a11y/href-no-hash' was not found jsx-a11y/href-no-hash
/private/tmp/demo-15/src/index.js
1:1 warning Definition for rule 'jsx-a11y/href-no-hash' was not found jsx-a11y/href-no-hash
/private/tmp/demo-15/src/registerServiceWorker.js
1:1 warning Definition for rule 'jsx-a11y/href-no-hash' was not found jsx-a11y/href-no-hash
✖ 4 problems (0 errors, 4 warnings)

[maciej-ka]

Adding plugins: ['jxs-a11y'] throws `Error: Failed to load plugin jxs-a11y: Cannot find module 'eslint-plugin-jxs-a11y' (after yarn eject)

And trying these options didn't help

 cwd: '/tmp/demo-15',
rulePaths: [
path.dirname(require.resolve('eslint-plugin-jsx-a11y')) + '/rules'
],

I wonder, why having baseConfig: { extends: "react-app" } in scripts/lint.js would be better than an explicit config file in the config directory? Both of these duplicate entry in package.json.

[viankakrisna]

i think this line can just be

require.resolve('eslint-config-react-app')

[viankakrisna]

after eject it needs to be

require.resolve(paths.appPackageJson),

though

[viankakrisna]

i think we can do it by playing around with eject comments 😅

// @remove-on-eject-begin/** * Copyright (c) 2015-present, Facebook, Inc. * * This source code is licensed under the MIT license found in the * LICENSE file in the root directory of this source tree. */// @remove-on-eject-end"use strict";constspawn=require("react-dev-utils/crossSpawn");constargs=process.argv.slice(2);// @remove-on-eject-begin/**// @remove-on-eject-endconst paths = require("../config/paths");// @remove-on-eject-begin*/// @remove-on-eject-endconstresult=spawn.sync("node",[require.resolve("eslint/bin/eslint")].concat(["src/**/*.{js,jsx,mjs}","--config",// @remove-on-eject-begin/** // @remove-on-eject-end require.resolve(paths.appPackageJson), // @remove-on-eject-begin */// @remove-on-eject-end// @remove-on-eject-beginrequire.resolve("eslint-config-react-app")// @remove-on-eject-end]).concat(args),{stdio: "inherit"});process.exit(result.status);

[viankakrisna]

or just

// @remove-on-eject-begin/** * Copyright (c) 2015-present, Facebook, Inc. * * This source code is licensed under the MIT license found in the * LICENSE file in the root directory of this source tree. */// @remove-on-eject-end"use strict";constspawn=require("react-dev-utils/crossSpawn");constargs=process.argv.slice(2);constresult=spawn.sync("node",[require.resolve("eslint/bin/eslint")].concat(["src/**/*.{js,jsx,mjs}",// @remove-on-eject-begin"--config",require.resolve("eslint-config-react-app")// @remove-on-eject-end]).concat(args),{stdio: "inherit"});process.exit(result.status);

[maciej-ka]

Thank you so much! Suggestion to userequire.resolve('eslint-config-react-app') as a config path is a gold.

[viankakrisna]

hardcoding this won't respect the changes that user made in package.json or .eslintrc post eject. IMO we should wrap this line:

// @remove-on-eject-beginconfigFile: require.resolve('eslint-config-react-app'),// @remove-on-eject-end

[maciej-ka]

yes ... but then react-scripts lint after eject has few errors:

/private/tmp/demo20/src/App.js
1:1 warning Definition for rule 'jsx-a11y/href-no-hash' was not found jsx-a11y/href-no-hash
/private/tmp/demo20/src/App.test.js
1:1 warning Definition for rule 'jsx-a11y/href-no-hash' was not found jsx-a11y/href-no-hash
/private/tmp/demo20/src/index.js
1:1 warning Definition for rule 'jsx-a11y/href-no-hash' was not found jsx-a11y/href-no-hash
/private/tmp/demo20/src/registerServiceWorker.js
1:1 warning Definition for rule 'jsx-a11y/href-no-hash' was not found jsx-a11y/href-no-hash
✖ 4 problems (0 errors, 4 warnings)

[viankakrisna]

hmm, do you have eslint installed globally / .eslintrc file somewhere in the parent folder? i think we need to add root: true to eslintConfig property in package.json post eject. https://eslint.org/docs/user-guide/configuring#using-configuration-files can you try adding it and see if it fix the problem?

[maciej-ka]

Sadly, no change after:

 "eslintConfig": {
"root": true,
"extends": "react-app"
}

[maciej-ka]

Also adding plugins: ['jsx-a11y'] to CLIEngine constructor options doesn't remove errors.

[viankakrisna]

maybe running it with DEBUG=eslint:* yarn lint would help you found the problem. I cannot reproduce it locally using the same code :(

[viankakrisna]

or DEBUG=eslint:config* yarn lint if you just want the log related to config

[maciej-ka]

E2E tests added, removed eslint config file. Huge thanks and ❤️ to @viankakrisna

[viankakrisna]

happy to help! so do you find what is the cause on the issue that you have on your machine? how do we avoid ejected users have the same issue as you?

[maciej-ka]

I haven't found the reason why this version of react-scripts lint is breaking, after eject, on my local dev. I had ESLint installed globally but it didn't help when I removed it. I hope to find some time to debug ESLint internals.

[viankakrisna]

what's the output of DEBUG=eslint:config* yarn lint ?

[maciej-ka]

Ha, it works now. Even when I install ESLinter globally.
Maybe it was some silly mistake from my side yesterday (like using a wrong version of react-scripts).

[sbdchd]

Might be a good idea to trigger process.exit(1); if there are errors, since the eslint CLIEngine won't do it itself.

[maciej-ka]

That would allow chaining yarn lint && .... Let's add it. I will return process.exit(1) if there was any linting error (but not a warning). Thanks for the idea!

[maciej-ka]

Condition to trigger exit(1) could be different when a --fix flag was used.

Report object has a count of fixable errors and it is possible to count how many unfixed errors are left:

{'...': '...',errorCount: 1,fixableErrorCount: 1,}

My concern is that in react-scripts lint --fix && git push files will be changed by lint but they still need git commit.

[uLan08]

Any updates on this?

[SirBif]

I'm trying to understand what's missing to close this PR. I was checking if I could help, but I have the impression this is just a rebase away from being ready.

• the e2e tests requested by @gaearon are there
• there is a merge conflict (which I believe is what's causing appveyor CI to fail)

Did I get that right @maciej-ka ?

[kfei]

Is this PR rejected?