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

Upgrade babel-eslint #7594

Closed
wants to merge 1 commit into from
Closed

Upgrade babel-eslint #7594

wants to merge 1 commit into from

Conversation

esetnik
Copy link

@esetnik esetnik commented Aug 26, 2019

@facebook-github-bot
Copy link

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
Copy link

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

@avindra
Copy link

avindra commented Aug 26, 2019

Sorry if this is the wrong place for this comment, but is there a reason why these versions don't have a caret?

Is there a documented reason for create-react-app pinning dependencies?

@esetnik
Copy link
Author

esetnik commented Aug 26, 2019

Sorry if this is the wrong place for this comment, but is there a reason why these versions don't have a caret?

Is there a documented reason for create-react-app pinning dependencies?

@avindra I was wondering the same thing. Happy to change this PR to a caret if there's no good reason why it is pinned.

@yyfearth
Copy link

Sorry if this is the wrong place for this comment, but is there a reason why these versions don't have a caret?
Is there a documented reason for create-react-app pinning dependencies?

@avindra I was wondering the same thing. Happy to change this PR to a caret if there's no good reason why it is pinned.

Actually the whole problem is caused by the caret ^.
Currently the eslint used ^, while babel-eslint pinned.

If both pinned, the issue won't occur since babel-eslint@10.0.2 works fine with eslint@6.1.0
The issue is babel-eslint@10.0.2 won't work with eslint@6.2.*

So if both use ^, the problem will be automatically fixed after babel-eslint@10.0.3 released, while user only have to update the lock.

@esetnik @avindra Beside from the issue itself, use ^ or pinned version, you cannot say which way is better.
Because not all packages in your node_modules always respect semver. If one of your dependency of dependencies has a broken patch or minor update, your whole project stop working or not working properly. And it really hard to debug and spot the issue.
Even with lockfile, we cannot ensure everything will work properly after update any dependency which will change the lockfile.
For projects with lots of dependencies, sometime we have no better way to avoid these, which result in pin all the versions of dependencies.

@silverwind
Copy link

silverwind commented Aug 26, 2019

I suggest also pinning eslint to exact version. It's not much work to update dependencies once in a while and much better than randomly broken builds because of breaking in-range upgrades like in this case.

@yyfearth
Copy link

Well, beside discussion, I think we should merge this PR and solve the problem ASAP, since it blocks people to upgrade to the new version just because of false positive eslint issues, which could failed the build in CI env.

@yyfearth
Copy link

Any update on this one? Waiting for merge and a new release.

@yyfearth
Copy link

I suggest also pinning eslint to exact version. It's not much work to update dependencies once in a while and much better than randomly broken builds because of breaking in-range upgrades like in this case.

@silverwind It won't take much work, but it take time to release. As you can see, this PR has not been merged yet. And even after this PR been merged, it will only be fixed in the next release. Which means, somebody using it may blocked for days or weeks.

@vincerubinetti
Copy link

vincerubinetti commented Aug 30, 2019

I second @yyfearth on the urgency of this one.

I could be wrong, but the cleanest way I could find to temporarily fix this issue was to add a pinned old version of eslint under dependencies, peerDependencies, and devDependencies, as well as a pinned old version of react-scripts. #7566 (comment) I find this very dirty though and feel that it should be unnecessary.

I can't believe other people aren't pushing for this more. I wonder what other people have done to temporarily solve this, and if they've done something similar to what I have, why they think that's okay.

@JustFly1984
Copy link

I always remove carret from all dependencies to pin the version, and considering it as good proctice, which is really saves time on resolving consequent deploys to dev prod an local environments with yarn.

@yyfearth
Copy link

yyfearth commented Sep 4, 2019

Still no action on this one? Blocked for weeks.

@tremby
Copy link

tremby commented Sep 4, 2019

How is a linter warning blocking you, @yyfearth?

@yyfearth
Copy link

yyfearth commented Sep 5, 2019

How is a linter warning blocking you, @yyfearth?

@tremby It blocks me to upgrade CRA from 3.0.1 to 3.1.1 only because the eslint change, which produce tons of false positive warnings.
We use CI to build the most of our CRA apps, and eslint warnings will fail the build by default (due to CI env).
Yes, we can turn the CI env off so the build will pass, but it still pollute the build logs, as well as lots of warnings in IDE and console log in browser during dev, which is very annoying and hide the real issues from the large amount of false ones.

@avindra
Copy link

avindra commented Sep 7, 2019

@yyfearth if you are using yarn, you can specify resolutions in package.json to override the version being resolved.

You'll also need to set the SKIP_PREFLIGHT_CHECK=true in your env, otherwise create-react-app will fail your build on an unexpected version being found.

Copy link

@patilharshal16 patilharshal16 left a comment

Choose a reason for hiding this comment

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

Is it support migration with all other libraries?

@ianschmitz
Copy link
Contributor

Thanks for the PR @esetnik. I merged the same fix as part of fixing our CI build in #7662. We'll get it out in the next patch release.

@ianschmitz ianschmitz closed this Sep 9, 2019
@lock lock bot locked and limited conversation to collaborators Sep 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

eslint for...in / for...of false positive
10 participants