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

Update eslint-plugin-react and enable new rules #696

Merged
merged 2 commits into from
Sep 22, 2016

Conversation

fson
Copy link
Contributor

@fson fson commented Sep 21, 2016

Some new rules had been added that are a good fit for this project. All of these are either already runtime warnings in React (no-danger-with-children, style-prop-object) or React features that are going to be deprecated in the future (no-find-dom-node). The new rules:

New rules:
* `react/no-danger-with-children` (jsx-eslint/eslint-plugin-react#710)
* `react/no-find-dom-node` (jsx-eslint/eslint-plugin-react#678)
* `react/style-prop-object` (jsx-eslint/eslint-plugin-react#715)
@ghost ghost added the CLA Signed label Sep 21, 2016
@fson
Copy link
Contributor Author

fson commented Sep 21, 2016

I also considered the new no-unused-prop-types rule, but wasn't sure if it would be a good fit here. Any thoughts on it @gaearon?

@ghost ghost added the CLA Signed label Sep 21, 2016
@gaearon
Copy link
Contributor

gaearon commented Sep 22, 2016

I think no-find-dom-node is a bit too early for this kit right now. It's okay for more mature setups but I'm worried beginners will copy and paste examples from StackOveflow and be confused by this warning.

I'm open to adding it in a year or so.

@fson
Copy link
Contributor Author

fson commented Sep 22, 2016

Alright, I removed no-find-dom-node. I think to be included in a beginner friendly configuration like CRA, the error message would also need to be better than the current Do not use findDOMNode, at least explain that you should use refs instead and possibly link to docs.

@gaearon
Copy link
Contributor

gaearon commented Sep 22, 2016

Exactly. (I'd appreciate if somebody did that.)

@gaearon
Copy link
Contributor

gaearon commented Sep 22, 2016

Is Travis acting up?

@fson
Copy link
Contributor Author

fson commented Sep 22, 2016

npm ERR! peerinvalid The package [email protected] does not satisfy its siblings' peerDependencies requirements!
npm ERR! peerinvalid Peer [email protected] wants [email protected]

Uh, it installs [email protected] from npm that still has the old [email protected] as a peer dependency. I guess we shouldn't change the ESLint plugin versions in react-scripts until we publish eslint-config-react-app with the new version? But then it fails when installing locally, because our local config depends on the new version... Tricky 😅

@gaearon
Copy link
Contributor

gaearon commented Sep 22, 2016

e2e script should use current versions of all packages, not ones from npm. Can we make it do that? Maybe need to run lerna bootstrap or whatever?

@gaearon
Copy link
Contributor

gaearon commented Sep 22, 2016

I’m going to trust that this works but we need to figure out the e2e fix for this.

@gaearon gaearon merged commit 2ed2b59 into facebook:master Sep 22, 2016
@gaearon gaearon added this to the 0.5.0 milestone Sep 22, 2016
@fson fson deleted the update-eslint-plugin-react branch September 22, 2016 12:59
@gaearon
Copy link
Contributor

gaearon commented Sep 22, 2016

#698

feiqitian pushed a commit to feiqitian/create-react-app that referenced this pull request Oct 25, 2016
* Update eslint-plugin-react and enable new rules

New rules:
* `react/no-danger-with-children` (jsx-eslint/eslint-plugin-react#710)
* `react/no-find-dom-node` (jsx-eslint/eslint-plugin-react#678)
* `react/style-prop-object` (jsx-eslint/eslint-plugin-react#715)

* Remove react/no-find-dom-node for now
@lock lock bot locked and limited conversation to collaborators Jan 22, 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.

2 participants