Update React to 16.3 #18768
Update React to 16.3 #18768
Conversation
💔 Build Failed |
💚 Build Succeeded |
|
@timroes At the moment, EUI and Kibana are both using 16.2. Since we're relying on EUI so heavily, and more every day, we really should be bumping EUI as well to ensure compatibility. |
|
We're tracking this in elastic/eui#763. |
| @@ -118,8 +118,9 @@ module.exports = { | |||
| 'react/jsx-first-prop-new-line': ['error', 'multiline-multiprop'], | |||
| 'react/jsx-equals-spacing': ['error', 'never'], | |||
| 'react/jsx-indent': ['error', 2], | |||
| 'react/no-did-mount-set-state': 'error', | |||
| 'react/no-did-update-set-state': 'error', | |||
| // Off as of React 16.3, see https://github.com/yannickcr/eslint-plugin-react/issues/1754 | |||
There was a problem hiding this comment.
This would be a good thing to add:
{
"settings": {
"react": {
"version": "16.3"
}
}
}It sounds like the solution upstream may be to noop those rules based on the React pragma version.
Seems like the actionable thing to do here is, when the React version pragma is >= v16.3, these rules should no-op.
There was a problem hiding this comment.
I added that react version to the .eslintrc config in Kibana, since the eslint-config-kibana actually doesn't have any knowledge about the React version Kibana is using.
| 'react/no-did-update-set-state': 'error', | ||
| // Off as of React 16.3, see https://github.com/yannickcr/eslint-plugin-react/issues/1754 | ||
| 'react/no-did-mount-set-state': 'off', | ||
| 'react/no-did-update-set-state': 'off', |
There was a problem hiding this comment.
Is there a reason these need to be set to off and not just removed?
There was a problem hiding this comment.
No. I thought we extend the recommended config, but since we didn't we can simply remove them :-)
| @@ -1,6 +1,6 @@ | |||
| { | |||
| "name": "@elastic/eslint-config-kibana", | |||
| "version": "0.15.0", | |||
| "version": "0.16.0", | |||
There was a problem hiding this comment.
Unless you're publishing this to NPM, please don't increment the version number
💚 Build Succeeded |
|
@cjcenizal good to see this. So the only important step is, that we merge this PR before the third step of your EUI plan. The first two steps are completly decoupled and won't benefit from updating or not updating via this PR. |
* Update React to 16.3 * Switch off specific eslint rules * Update enzyme snapshots * Incorporate PR feedback
| @@ -118,8 +118,6 @@ module.exports = { | |||
| 'react/jsx-first-prop-new-line': ['error', 'multiline-multiprop'], | |||
| 'react/jsx-equals-spacing': ['error', 'never'], | |||
| 'react/jsx-indent': ['error', 2], | |||
| 'react/no-did-mount-set-state': 'error', | |||
| 'react/no-did-update-set-state': 'error', | |||
There was a problem hiding this comment.
Not related to the React bump but thought I'd put it here:
Are we still trying to follow airbnb's convention (with minor modifications) or do we see our style guide as very different?
If we still intent to follow airbnb style, wouldn't it be nicer if we extended their rules? Their react rules look pretty similar:
https://github.com/airbnb/javascript/blob/master/packages/eslint-config-airbnb/rules/react.js
This PR just updates React to 16.3 for Kibana and x-pack.
I already have my Inspector branch build upon 16.3 and just wanted to merge that in at some point in time. But since that feature might take another few weeks and 16.3 brings some important lifecycle changes, I wanted to create a separate PR for this update.
That way people who are currently developing React components could already make use of the new life-cycle methods, before I merged the Inspector. Also once this is merged, I could write a mail with information about the new lifecycle and make devs aware of #17432. With every week we don't update this, we potentially create new React components, that will later need refactoring again, so I hope we can do this update as soon as possible.
This also turns of two eslint rules, that will anyway be noop in the future for React 16.3 according to jsx-eslint/eslint-plugin-react#1754.
Also I updated one snapshot, that was failing due to the update, because React 16.3, doesn't assign the
value="on"for checkboxes, but therefore avalue=""for empty managed text fields by default.