added react-scripts lint #2729

devonjs · 2017-07-06T08:17:02Z

Added react-script for linting with specifications in #1217 & #2625

facebook-github-bot · 2017-07-06T08:17:24Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

facebook-github-bot · 2017-07-06T08:28:55Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

gaearon

See comments above.

It would also be great to add a step verifying react-scripts lint works before and after ejecting as part of "simple" end-to-end test.

gaearon · 2017-07-06T12:16:29Z

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

+    spawn.sync(
+      'node',
+      [
+        'node_modules/eslint/bin/eslint.js',


I would prefer that we use require.resolve here rather than hardcode node_modules path.

gaearon · 2017-07-06T12:16:32Z

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

+      [
+        'node_modules/eslint/bin/eslint.js',
+        '--config',
+        'node_modules/eslint-config-react-app/index.js',


devonjs · 2017-07-06T21:58:24Z

@gaearon updated PR (rebased) with your notes for require.resolve and simple e2e test.

Found that on the close event listener, ESLint will send a code of 0 or 1 on successful finishes (0: ESLint found no lint errors, 1: ESLint found lint errors) so that code is not particularly useful. So what we can only mainly check is errors throughout and the closing signal.

gaearon · 2017-07-12T15:01:33Z

Apologies, what I meant is we should add ./node_modules/.bin/react-scripts lint to our end-to-end testing suite that runs on CI. This helps us catch regressions. We should run it both before and after ejecting.

devonjs · 2017-07-18T17:17:28Z

@gaearon updated commit, let me know what you think

devonjs · 2017-08-01T19:55:54Z

@gaearon ping!

Timer · 2017-08-02T01:44:20Z

Hmm, I'm really not sold on the benefit here. This can be handled on CI way more accurately as npm run build or as part of npm test.

I'll leave it to your discretion, @gaearon.

devonjs · 2017-08-18T19:23:40Z

@Timer yeah, it's a bit weird to really test if linting works or not. Putting it in tasks/e2e-simple.sh made sense to me because it tries to lint and if it doesn't work, it throws the error.

Is there a better way in the npm run build or npm test script that can test if lint works? Perhaps I might be thinking of the test wrong (lint works or not), when there's a better way to test it!

gaearon · 2017-08-18T22:58:01Z

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

+      console.log('ESLint finished');
+    });
+    results.on('error', err => {
+      console.log('Error running ESLint: ' + err);


This should exit with an exit code. Otherwise CI wouldn't fail.

gaearon · 2017-08-18T22:58:27Z

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

+        console.log('ESLint closed with signal: ' + signal);
+        return;
+      }
+      console.log('ESLint finished');


I don't think we need to log this.

gaearon · 2017-08-18T22:59:35Z

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

+      [
+        require.resolve('eslint/bin/eslint'),
+        '--config',
+        require.resolve('eslint-config-react-app'),


After ejecting this should somehow let it infer config from package.json.

gaearon · 2017-08-18T23:00:30Z

This can be handled on CI way more accurately as npm run build or as part of npm test.

This is for the case when build is taking minutes and you just want to do a quick lint check.

devonjs · 2017-09-05T02:48:25Z

@gaearon updated, apologies in advance for a bit of hand-holding through this

react-scripts-dangerous · 2017-09-29T03:22:00Z

Hello! I'm a bot that helps facilitate testing pull requests.

Your pull request (commit a452a0f) has been released on npm for testing purposes.

npm i [email protected]
# or
yarn add [email protected]
# or
create-react-app [email protected] folder/

Note that the package has not been reviewed or vetted by the maintainers. Only install it at your own risk!

Thanks for your contribution!

Stupidism · 2017-10-23T07:48:17Z

How is this going?

RWOverdijk · 2017-10-23T08:14:59Z

For anyone interested, I solved this differently because I have some other code styles to follow. It's really not that challenging:

Create an .eslintrc file:

{
  "extends": "./node_modules/eslint-config-react-app/index.js",
  "rules"  : {
    "key-spacing": [
      "error",
      {
        "align": "colon"
      }
    ]
  }
}

Add a linting script in package.json:

"lint": "node_modules/eslint/bin/eslint.js src"

That's it. Now you cna run yarn lint or npm run lint. And you can define your own rules.

trevordmiller · 2018-01-14T01:11:22Z

Being able to run react-scripts lint like react-scripts test and react-scripts start would be very helpful for both CI and pre-commit hooks.

gaearon · 2018-01-14T01:26:29Z

Agreed, we want to take this in some form. It's been a while so I don't quite remember why this PR does things in the way it does. Let me leave a comment.

gaearon

See below: I don't quite understand why this is written with a try-catch, and I don't think it will work after ejecting.

gaearon · 2018-01-14T01:26:44Z

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

+
+    try {
+      eslintConfigPath = require.resolve('eslint-config-react-app');
+    } catch (e) {


When would this throw and why? I don't understand.

gaearon · 2018-01-14T01:26:53Z

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

+    try {
+      eslintConfigPath = require.resolve('eslint-config-react-app');
+    } catch (e) {
+      eslintConfigPath = require.resolve('../../../package.json');


Why would package.json count as a valid config?

gaearon · 2018-01-14T01:27:55Z

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

+        require.resolve('eslint/bin/eslint'),
+        '--config',
+        eslintConfigPath,
+        'src/**/*.{js,jsx}',


Need to include .mjs now that we support it.

It's interesting that this doesn't filter tests out. I guess we can say it's a feature. (Currently there's no way to lint tests.)

gaearon · 2018-01-14T01:30:00Z

tasks/e2e-simple.sh

@@ -314,6 +317,9 @@ npm link "$root_path"/packages/eslint-config-react-app
 npm link "$root_path"/packages/react-dev-utils
 npm link "$root_path"/packages/react-scripts

+# Test lint
+"$root_path"/packages/react-scripts/bin/react-scripts.js lint


This doesn't make sense to me. What is this testing?

After you eject, react-scripts doesn't exist (it might on CI but you shouldn't rely on this—the point of ejecting is to wipe out react-scripts on the next installs).

So the script clearly doesn't "still work" after ejecting. If you had "lint": "react-scripts lint" before ejecting, it will stop working after.

gaearon · 2018-01-14T01:32:03Z

@devonjs Thanks for your initial work with this! I think this PR might need somebody’s helping had to push this through. I’m sorry I didn’t get time to review it in more detail earlier.

@trevordmiller If you want to pick this up given the above review and resubmit it with these points addressed we can definitely try to get it in!

maciej-ka · 2018-01-17T18:07:51Z

I would give a try to close this. Should I open a new PR to submit changes?

trevordmiller · 2018-01-17T18:26:40Z

@gaearon Thanks! Sorry, I'm currently swamped or else I would. I appreciate the invite though. @devonjs or @maciej-ka seem like they could maybe do it?

gaearon · 2018-01-17T19:00:42Z

@maciej-ka Sounds good!

techrah · 2018-01-19T21:57:35Z

Please provide link to new PR. Thanks! :)

maciej-ka · 2018-01-19T22:35:06Z

#3850

SirBif · 2018-10-03T13:34:42Z

Should this PR be closed since it has been picked up and continued in #3850 ? @Timer

facebook-github-bot added the CLA Signed label Jul 6, 2017

devonjs mentioned this pull request Jul 6, 2017

Run Lint from the command line? #1217

Open

gaearon suggested changes Jul 6, 2017

View reviewed changes

devonjs force-pushed the master branch from 60fa35c to ca52751 Compare July 6, 2017 21:52

devonjs force-pushed the master branch from ca52751 to d20e048 Compare July 18, 2017 09:35

Timer mentioned this pull request Aug 2, 2017

eslint command added to package.json #2625

Closed

gaearon suggested changes Aug 18, 2017

View reviewed changes

added react-scripts lint & e2e test

a452a0f

devonjs force-pushed the master branch from d20e048 to a452a0f Compare September 5, 2017 00:34

Timer added this to the 1.x milestone Sep 29, 2017

frinyvonnick mentioned this pull request Nov 6, 2017

🔧 lerna unirakun/hoc-react-loader#82

Merged

gaearon suggested changes Jan 14, 2018

View reviewed changes

gaearon modified the milestones: 1.x, 2.0.0 Jan 14, 2018

gaearon mentioned this pull request Jan 16, 2018

Roadmap for [email protected] #3815

Closed

JaKXz mentioned this pull request Jan 16, 2018

Add stylelint #1216

Closed

marcofugaro mentioned this pull request Feb 19, 2018

Add linting scripts for use in CI accurat/accurapp#2

Closed

Timer modified the milestones: 2.0.x, 2.x Sep 26, 2018

Timer removed this from the 2.x milestone Nov 2, 2018

josgraha mentioned this pull request Nov 19, 2018

continue #2729 and only add react-scripts lint #5851

Closed

ianschmitz closed this Feb 7, 2019

lock bot locked and limited conversation to collaborators Feb 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added react-scripts lint #2729

added react-scripts lint #2729

devonjs commented Jul 6, 2017

facebook-github-bot commented Jul 6, 2017

facebook-github-bot commented Jul 6, 2017

gaearon left a comment

gaearon Jul 6, 2017

gaearon Jul 6, 2017

devonjs commented Jul 6, 2017

gaearon commented Jul 12, 2017

devonjs commented Jul 18, 2017

devonjs commented Aug 1, 2017

Timer commented Aug 2, 2017

devonjs commented Aug 18, 2017

gaearon Aug 18, 2017

gaearon Aug 18, 2017

gaearon Aug 18, 2017

gaearon commented Aug 18, 2017

devonjs commented Sep 5, 2017

react-scripts-dangerous commented Sep 29, 2017

Stupidism commented Oct 23, 2017

RWOverdijk commented Oct 23, 2017

trevordmiller commented Jan 14, 2018 •

edited

Loading

gaearon commented Jan 14, 2018

gaearon left a comment •

edited

Loading

gaearon Jan 14, 2018

gaearon Jan 14, 2018

gaearon Jan 14, 2018

gaearon Jan 14, 2018

gaearon commented Jan 14, 2018

maciej-ka commented Jan 17, 2018

trevordmiller commented Jan 17, 2018

gaearon commented Jan 17, 2018

techrah commented Jan 19, 2018

maciej-ka commented Jan 19, 2018

SirBif commented Oct 3, 2018

added react-scripts lint #2729

added react-scripts lint #2729

Conversation

devonjs commented Jul 6, 2017

facebook-github-bot commented Jul 6, 2017

facebook-github-bot commented Jul 6, 2017

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

devonjs commented Jul 6, 2017

gaearon commented Jul 12, 2017

devonjs commented Jul 18, 2017

devonjs commented Aug 1, 2017

Timer commented Aug 2, 2017

devonjs commented Aug 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon commented Aug 18, 2017

devonjs commented Sep 5, 2017

react-scripts-dangerous commented Sep 29, 2017

Stupidism commented Oct 23, 2017

RWOverdijk commented Oct 23, 2017

trevordmiller commented Jan 14, 2018 • edited Loading

gaearon commented Jan 14, 2018

gaearon left a comment • 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

gaearon commented Jan 14, 2018

maciej-ka commented Jan 17, 2018

trevordmiller commented Jan 17, 2018

gaearon commented Jan 17, 2018

techrah commented Jan 19, 2018

maciej-ka commented Jan 19, 2018

SirBif commented Oct 3, 2018

trevordmiller commented Jan 14, 2018 •

edited

Loading

gaearon left a comment •

edited

Loading