added react-scripts lint by devonjs · Pull Request #2729 · react/create-react-app

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 cla@fb.com. 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

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

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

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

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

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

I don't think we need to log this.

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

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 react-scripts-dangerous@1.0.11-a452a0f.0
# or
yarn add react-scripts-dangerous@1.0.11-a452a0f.0
# or
create-react-app --scripts-version=react-scripts-dangerous@1.0.11-a452a0f.0 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

+
+ 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

+ 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

+ 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

 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 ca52751Compare July 6, 2017 21:52

devonjs force-pushed the master branch from ca52751 to d20e048Compare 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 a452a0fCompare 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.0Jan 14, 2018

gaearon mentioned this pull request Jan 16, 2018
Roadmap for react-scripts@2.0 #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

lockBot locked and limited conversation to collaborators Feb 12, 2019

Uh oh!

Uh oh!

Conversation

devonjs commented Jul 6, 2017

Uh oh!

facebook-github-bot commented Jul 6, 2017

Uh oh!

facebook-github-bot commented Jul 6, 2017

Uh oh!

gaearon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devonjs commented Jul 6, 2017

Uh oh!

gaearon commented Jul 12, 2017

Uh oh!

devonjs commented Jul 18, 2017

Uh oh!

devonjs commented Aug 1, 2017

Uh oh!

Timer commented Aug 2, 2017

Uh oh!

devonjs commented Aug 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaearon commented Aug 18, 2017

Uh oh!

devonjs commented Sep 5, 2017

Uh oh!

react-scripts-dangerous commented Sep 29, 2017

Uh oh!

Stupidism commented Oct 23, 2017

Uh oh!

RWOverdijk commented Oct 23, 2017

Uh oh!

trevordmiller commented Jan 14, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaearon commented Jan 14, 2018

Uh oh!

gaearon left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaearon commented Jan 14, 2018

Uh oh!

maciej-ka commented Jan 17, 2018

Uh oh!

trevordmiller commented Jan 17, 2018

Uh oh!

gaearon commented Jan 17, 2018

Uh oh!

techrah commented Jan 19, 2018

Uh oh!

maciej-ka commented Jan 19, 2018

Uh oh!

SirBif commented Oct 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

trevordmiller commented Jan 14, 2018•
edited
Loading

gaearon left a comment •
edited
Loading