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

chore: Allow to install eslint-plugin-react ^7.1.0 to get rid of warnings on start #3087

Closed
wants to merge 1 commit into from

Conversation

FezVrasta
Copy link
Contributor

This should allow people to install eslint-plugin-react 7.2.0 which gets rid of the following warnings when you run yarn start:

can't resolve reference #/definitions/basicConfig from id #
can't resolve reference #/definitions/basicConfigOrBoolean from id #
can't resolve reference #/definitions/basicConfigOrBoolean from id #

@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!

@Timer
Copy link
Contributor

Timer commented Sep 7, 2017

What warnings are you talking about; could you please give me more context? These are development dependencies of a required package, so they should never be installed under normal use anyway.

@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!

@FezVrasta
Copy link
Contributor Author

FezVrasta commented Sep 7, 2017

So, when you start the app, you get the warnings reported in my first post.

The problem is that yarn (and probably npm) will install 7.1.0 instead of any newer version because this is the only version that matches the requirements of all the packages.

If you loosen the requirement here, the latest version will be installed and used by all the other packages of create-react-app as well.

image

❯ cat node_modules/eslint-plugin-react/package.json | grep version
  "version": "7.1.0",

If I put this in my package.json:

  "resolutions": {
    "eslint-plugin-react": "7.2.0"
  }

The warnings go away

@Timer
Copy link
Contributor

Timer commented Sep 7, 2017

I guess more appropriately, I cannot reproduce this.
A demo would be useful.

Are you using Yarn, npm? What version? What react-scripts version (I assume latest; or is this the monorepo)?

Once again, you're modifying a development dependency which should not affect the install tree (ever); did you mean to edit the package in react-scripts?

@FezVrasta
Copy link
Contributor Author

FezVrasta commented Sep 7, 2017

Oh, yeah you are right, I should have edited the one in react-scripts.

The warnings comes out after you eject and thinker a bit with the app, but the base problem is that react-scripts requires an older version without any particular reason.

@Timer
Copy link
Contributor

Timer commented Sep 7, 2017

react-scripts strictly depends on pinned versions of packages to prevent accidental breaking changes in package releases -- a common occurrence in the tooling world.

I do not see us loosening this requirement as it's not clear to me how this benefits anything and how it couldn't be simply overridden when you eject (or how it causes this problem).

If there's a problem here, it sounds like it's more in-line with invalid configuration or a breaking change released under a non-major version (and then stemming from configuration in eslint-config-react-app).

@FezVrasta
Copy link
Contributor Author

FezVrasta commented Sep 7, 2017

Can't we bump it to 7.2.0 then? It simply fixes this warning, can't be anything bad.

@Timer
Copy link
Contributor

Timer commented Sep 7, 2017

I would like to figure out the underlying cause before we update packages.

@adam187
Copy link

adam187 commented Oct 4, 2017

@Timer @FezVrasta

I had similar issue.
In my case this warning did show when i was linting app with my own eslint config.
my external config have eslint-plugin-react@7.4.0 as dependency but because react-scripts requires eslint-plugin-react@7.1.0 this was version that npm installed.

adding

"resolutions": {
    "eslint-plugin-react": "7.4.0"
  }

to my package.json solved issue.

But i didn't get this warring while starting server via yarn start

@react-scripts-dangerous
Copy link

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

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

npm i react-scripts-dangerous@1.0.14-d68fa77.0
# or
yarn add react-scripts-dangerous@1.0.14-d68fa77.0
# or
create-react-app --scripts-version=react-scripts-dangerous@1.0.14-d68fa77.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!

@gaearon
Copy link
Contributor

gaearon commented Oct 28, 2017

We will bump this plugin in the next major. It's not supported to install and use arbitrary versions of ESLint plugins in a non-ejected project.

@gaearon gaearon closed this Oct 28, 2017
@lock lock bot locked and limited conversation to collaborators Jan 20, 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.

6 participants