feat: react-scripts lint command #1217 #3850

maciej-ka · 2018-01-18T03:11:52Z

Adds lint command to react-scripts.

Typical use will be to:

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 · 2018-01-18T09:12:25Z

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

maciej-ka · 2018-01-18T18:42:32Z

Fixed and tested. Should work after ejecting now.

gaearon · 2018-01-18T19:08:40Z

packages/react-scripts/bin/react-scripts.js

@@ -55,6 +55,10 @@ switch (script) {
    process.exit(result.status);
    break;
  }
+  case 'lint': {


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

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 · 2018-01-18T19:09:25Z

packages/react-scripts/scripts/lint.js

+  { stdio: 'inherit' }
+);
+if (args.indexOf('--init') >= 0) {
+  console.log(


What is this for? I don't understand.

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.

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

maciej-ka · 2018-01-19T00:43:55Z

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 · 2018-01-20T00:39:17Z

@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 · 2018-01-20T00:57:47Z

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 · 2018-01-20T01:12:18Z

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

maciej-ka · 2018-01-20T11:14:44Z

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 · 2018-01-21T19:40:45Z

packages/react-scripts/config/eslint.json

@@ -0,0 +1,3 @@
+{
+  "extends": "react-app"
+}


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

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

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:

const CLIEngine = require("eslint").CLIEngine; const cli = new CLIEngine({ configFile: require.resolve('../config/eslint.json'), fix: process.argv.slice(2).indexOf('--fix') >= 0 }); const report = cli.executeOnFiles(["src/**/*.{js,jsx,mjs}"]); CLIEngine.outputFixes(report); const formatter = 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']

Can baseConfig argument help?

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)

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 · 2018-01-23T18:28:14Z

packages/react-scripts/scripts/lint.js

+    .concat([
+      'src/**/*.{js,jsx,mjs}',
+      '--config',
+      require.resolve('../config/eslint.json'),


i think this line can just be

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

after eject it needs to be

require.resolve(paths.appPackageJson),

though

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"; const spawn = require("react-dev-utils/crossSpawn"); const args = process.argv.slice(2); // @remove-on-eject-begin /** // @remove-on-eject-end const paths = require("../config/paths"); // @remove-on-eject-begin */ // @remove-on-eject-end const result = 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-begin require.resolve("eslint-config-react-app") // @remove-on-eject-end ]) .concat(args), { stdio: "inherit" } ); process.exit(result.status);

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"; const spawn = require("react-dev-utils/crossSpawn"); const args = process.argv.slice(2); const result = 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);

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

viankakrisna · 2018-01-23T21:42:19Z

packages/react-scripts/scripts/lint.js

+const CLIEngine = require('eslint').CLIEngine;
+
+const cli = new CLIEngine({
+  configFile: require.resolve('eslint-config-react-app'),


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-begin configFile: require.resolve('eslint-config-react-app'), // @remove-on-eject-end

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)

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?

Sadly, no change after:

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

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

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

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

maciej-ka · 2018-01-24T10:16:32Z

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

viankakrisna · 2018-01-24T10:23:09Z

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 · 2018-01-24T14:13:55Z

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 · 2018-01-24T14:17:09Z

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

maciej-ka · 2018-01-24T15:13:41Z

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 · 2018-01-30T19:03:40Z

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 · 2018-01-30T21:07:38Z

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 · 2018-01-30T21:36:46Z

packages/react-scripts/scripts/lint.js

+CLIEngine.outputFixes(report);
+console.log(formatter(report.results));
+
+if (report.errorCount > 0) {


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 · 2018-04-12T01:56:05Z

Any updates on this?

SirBif · 2018-10-03T13:29:45Z

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 · 2019-01-08T09:40:32Z

Is this PR rejected?

facebook-github-bot added the CLA Signed label Jan 18, 2018

gaearon suggested changes Jan 18, 2018

View reviewed changes

maciej-ka force-pushed the react-scripts-lint-command branch from a9c3973 to 6fe704b Compare January 18, 2018 18:35

maciej-ka force-pushed the react-scripts-lint-command branch from 6fe704b to 4c89a1f Compare January 18, 2018 18:44

gaearon reviewed Jan 18, 2018

View reviewed changes

maciej-ka force-pushed the react-scripts-lint-command branch from 4c89a1f to 27d65f9 Compare January 19, 2018 00:00

This was referenced Jan 19, 2018

Option to use yarn in init wizard eslint/eslint#9863

Closed

added react-scripts lint #2729

Closed

maciej-ka force-pushed the react-scripts-lint-command branch from 27d65f9 to 583132e Compare January 20, 2018 11:10

gaearon suggested changes Jan 21, 2018

View reviewed changes

viankakrisna reviewed Jan 23, 2018

View reviewed changes

maciej-ka force-pushed the react-scripts-lint-command branch 2 times, most recently from 7dc0f55 to 9998ca2 Compare January 23, 2018 21:09

viankakrisna reviewed Jan 23, 2018

View reviewed changes

maciej-ka force-pushed the react-scripts-lint-command branch 5 times, most recently from a28a0f8 to 7a5ab30 Compare January 24, 2018 08:44

maciej-ka force-pushed the react-scripts-lint-command branch 2 times, most recently from 2529a76 to 67a774d Compare January 30, 2018 21:21

maciej-ka commented Jan 30, 2018

View reviewed changes

maciej-ka force-pushed the react-scripts-lint-command branch from 67a774d to 7543ab4 Compare January 30, 2018 22:04

feat: react-scripts lint command facebook#1217

aec1ba1

maciej-ka force-pushed the react-scripts-lint-command branch from 7543ab4 to aec1ba1 Compare January 30, 2018 22:11

Timer closed this Sep 26, 2018

Timer reopened this Sep 26, 2018

Rosey mentioned this pull request Oct 5, 2018

Add eslint? draft-js-plugins/next#8

Closed

gaearon closed this Nov 1, 2018

lock bot locked and limited conversation to collaborators Jan 13, 2019

feat: react-scripts lint command #1217 #3850

feat: react-scripts lint command #1217 #3850

Conversation

maciej-ka commented Jan 18, 2018 • edited Loading

gaearon left a comment

Choose a reason for hiding this comment

maciej-ka commented Jan 18, 2018

maciej-ka commented Jan 18, 2018

Choose a reason for hiding this comment

maciej-ka Jan 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maciej-ka commented Jan 19, 2018

maciej-ka commented Jan 20, 2018

gaearon commented Jan 20, 2018

maciej-ka commented Jan 20, 2018

maciej-ka commented Jan 20, 2018

gaearon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maciej-ka Jan 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viankakrisna Jan 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viankakrisna Jan 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maciej-ka Jan 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maciej-ka commented Jan 24, 2018

viankakrisna commented Jan 24, 2018 • edited Loading

maciej-ka commented Jan 24, 2018

viankakrisna commented Jan 24, 2018

maciej-ka commented Jan 24, 2018

sbdchd commented Jan 30, 2018

maciej-ka commented Jan 30, 2018

Choose a reason for hiding this comment

uLan08 commented Apr 12, 2018

SirBif commented Oct 3, 2018

kfei commented Jan 8, 2019

maciej-ka commented Jan 18, 2018 •

edited

Loading

maciej-ka Jan 19, 2018 •

edited

Loading

maciej-ka Jan 21, 2018 •

edited

Loading

viankakrisna Jan 23, 2018 •

edited

Loading

viankakrisna Jan 23, 2018 •

edited

Loading

maciej-ka Jan 23, 2018 •

edited

Loading

viankakrisna commented Jan 24, 2018 •

edited

Loading