Updates react to 16.6 (latest) #24707

jasonrhodes · 2018-10-28T15:22:18Z

Summary

Upgrades the following packages to "latest" in order to keep our React dependencies up to date.

react
react-dom
enzyme
enzyme-adapter-react-16
enzyme-adapter-utils
enzyme-to-json

These deps have been updated in the kibana root as well as in the x-pack directory. I'm not sure if it needs to be upgraded in other places or if there is a kbn command to do these kinds of upgrades across the project.

NOTES:

This PR can replace Upgrade react and react-dom to 16.5.2 #23437 which was upgrading these dependencies to a now-older version. (ping @markov00)
Upgrading the enzyme packages resolved all but 2 of the jest unit test failures in the x-pack folder.
There are 42 failing snapshots in the kibana root jest run, but almost all of them are due to the fact that React no longer seems to render <React.Fragment> in snapshot tests, or in some cases renders as <Fragment> instead of <React.Fragment>.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~[ ] This was checked for cross-browser compatibility, including a check against IE11~~
~~[ ] Any text added follows [EUI's writing guidelines]~~(https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes i18n support
~~[ ] Documentation was added for features that require explanation or tutorials~~
Unit or functional tests were updated or added to match the most common scenarios
~~[ ] This was checked for keyboard-only and screenreader accessibility~~

For maintainers

~~[ ] This was checked for breaking API changes and was [labeled appropriately]~~(https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#release-notes-process)
~~[ ] This includes a feature addition or change that requires a release note and was labeled appropriately~~

elasticmachine · 2018-10-28T15:41:38Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

jasonrhodes · 2018-10-28T16:25:24Z

retest

elasticmachine · 2018-10-28T17:06:55Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

markov00 · 2018-10-29T09:17:35Z

Thanks @jasonrhodes for doing this. also @sqren was interested in doing this.

About the React.Fragment thing, yes it's a change in the enzyme adapter (but I cannot find a changelog that describe exactly what changed), I already seen this in the EUI.

The x-pack failures are a bit strange, seems that they are requiring the adapter but it's loaded via x-pack/dev-tools/jest/setup/enzyme.js.

sorenlouv · 2018-10-29T15:20:02Z

Can't wait for the hooks either? :D

jasonrhodes · 2018-10-30T03:15:50Z

Can't wait for the hooks either? :D

Suspennnnnnnnnse

spalger · 2018-11-03T00:27:33Z

I tried to do basically the same thing but was concerned by some of the snapshot changes I was seeing, which led me to find the change in getDerivedStateFromProps() behavior introduced in 16.4 and file several issues for components that will need to be updated before we can upgrade:

All other uses of getDerivedStateFromProps() were either compatible with calls before each render, or were in components that didn't use setState().

spalger · 2018-11-29T00:14:08Z

@jasonrhodes All those issues are closed now, want to take another shot at this upgrade?

elasticmachine · 2018-11-29T00:15:26Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

jasonrhodes · 2018-11-29T04:09:55Z

@spalger absolutely, will give this a shot tomorrow -- thanks for the heads up!

jasonrhodes · 2019-01-03T16:50:10Z

I rebased this against master and re-ran tests, here are the results. In the Kibana root:

Test Suites: 30 failed, 1 skipped, 490 passed, 520 of 521 total
Tests:       53 failed, 1 skipped, 3428 passed, 3482 total
Snapshots:   51 failed, 1017 passed, 1068 total

In x-pack:

Test Suites: 39 failed, 288 passed, 327 total
Tests:       70 failed, 1719 passed, 1789 total
Snapshots:   70 failed, 626 passed, 696 total

In both cases, these are primarily due to <React.Fragment> becoming <Fragment> in snapshots. I will go through and update all snapshots that are only failing because of this fragment change and report back about the remaining failures.

elasticmachine · 2019-01-03T17:37:20Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

jasonrhodes · 2019-01-03T20:46:38Z

After fixing/updating snapshots where <React.Fragment> changed to <Fragment> or, in some cases, disappeared, we still have about 35 failing tests for React 16.

For Kibana root:

Test Suites: 10 failed, 1 skipped, 510 passed, 520 of 521 total
Tests:       18 failed, 1 skipped, 3463 passed, 3482 total
Snapshots:   16 failed, 1052 passed, 1068 total

and for x-pack:

Test Suites: 12 failed, 315 passed, 327 total
Tests:       16 failed, 1773 passed, 1789 total
Snapshots:   16 failed, 680 passed, 696 total

So that's roughly 30 snapshots and 2 unit tests that need some review. A lot of those 30 snapshots are still related to <React.Fragment> but the diffs are so involved that I didn't want to just u them too easily, so I'd appreciate a couple more sets of eyes before mashing them through.

@spalger @chandlerprall @sqren @ogupte @markov00 or anyone else wanna have a go?

jasonrhodes · 2019-01-03T20:52:12Z

Also note: the "+12000" lines in the diff is almost entirely because the yarn.lock file was missing from the x-pack folder for some reason, and this PR added it back since I ran bootstrap to get the updated deps sorted. Not sure why it was missing?

legrego · 2019-01-03T20:57:19Z

^^ @joshdover did your work on yarn workspaces intentionally remove this? I seem to remember you mentioning something about it, but I might be making that up

legrego · 2019-01-03T21:01:05Z

...nt/edit_role/components/privileges/kibana/__snapshots__/impacted_spaces_flyout.test.tsx.snap

@@ -1,7 +1,7 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP

 exports[`<ImpactedSpacesFlyout> renders without crashing 1`] = `
-<React.Fragment>
+<Fragment>


question: Why do some of the snapshot differences change <React.Fragment> => <Fragment>, whereas others just remove the fragment altogether, like this example?

This is a good question. I looked into it a bit and couldn't find a solid answer. I looked at some of the source and nothing changed, it seemed like possibly when there is a single fragment inside of another single element, the fragment may disappear since it's "not needed" but I'm not totally sure if that's true.

Enzyme has removed fragments from rendered output since roughly 3.3.5 or 3.3.6 - https://github.com/airbnb/enzyme/blame/master/packages/enzyme/src/RSTTraversal.js#L25

Dug in a bit more; in cases where the Fragment is the top-level/ROOT/HOST node (as it is in ImpactedSpacesFlyout> renders without crashing 1), then the Fragment is never seen as a child so it doesn't get expanded.

elasticmachine · 2019-01-03T21:30:18Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

sorenlouv · 2019-01-05T19:06:41Z

Really awesome work on upgrading React! Would it make sense to bump it to the latest (16.7) now you are at it? 16.7 is supposedly fully backwards compatible with 16.6, and was only labelled as a minor release (instead of patch) to err on the side of caution.
https://reactjs.org/blog/2018/12/19/react-v-16-7.html

joshdover · 2019-01-07T15:23:51Z

@legrego @jasonrhodes Right, there should only be a single yarn.lock file now. If you had started this before yarn-workspaces was merged you probably got a conflict on the x-pack/yarn.lock. That file should be removed from your PR.

jasonrhodes · 2019-01-09T15:15:03Z

@legrego @jasonrhodes Right, there should only be a single yarn.lock file now. If you had started this before yarn-workspaces was merged you probably got a conflict on the x-pack/yarn.lock. That file should be removed from your PR.

But I think running yarn kbn bootstrap still creates it ... that's going to be a problem isn't it?

joshdover · 2019-01-09T15:18:38Z

@jasonrhodes That shouldn't be the case. If you delete the file and run bootstrap again, is it re-appearing?

jasonrhodes · 2019-01-09T15:20:17Z

@joshdover i'll try when I have a minute to get back on this branch. All I know is that the file was already missing for me when I rebased this branch, I believe, and running bootstrap to update the react deps seemed to re-create it. I'll try again and confirm, though.

joshdover · 2019-01-09T15:30:22Z

@jasonrhodes Definitely seems strange. We've updated other dependencies since workspaces went in and this hasn't happened. I think what happened here is that this work was started before workspaces went in and git may have put the file back when you merged/rebased with master.

If it does reappear after deleting/bootstrap again, lmk and I can help investigate.

…oduced by accident during a rebase in this branch

elasticmachine · 2019-01-18T23:07:16Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

jasonrhodes · 2019-01-22T22:42:04Z

@Bargs @chandlerprall this commit is pushed 4893cba

That fixes the issue with the query_bar blocking state updates due to the 16.4 changes with getDerivedStateFromProps running on every render and not just on prop updates. Give me a thumbs up if you take a look and think this solves the problem safely. Thanks!

elasticmachine · 2019-01-22T22:52:40Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-01-23T12:40:22Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-01-23T14:50:44Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

chandlerprall · 2019-01-23T15:59:08Z

@jasonrhodes same snapshot update needed for x-pack tests (executeQueryOptions prop appearing)

chandlerprall

Changes + snapshot updates look great

Bargs

Query bar change LGTM

elasticmachine · 2019-01-23T17:22:06Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

jasonrhodes · 2019-01-23T17:41:46Z

@joshdover / @legrego you all have commented here in the past so I'm pinging you for a last look/approval :D

legrego

LGTM!

jasonrhodes · 2019-01-23T18:15:15Z

@stacey-gammon do you want to look at anything here, with a React upgrade?

* Updates react to 16.6 (latest) * Updated fragment-based snapshots for Kibana root unit tests * Updated fragment-based snapshots for x-pack unit tests * Removed xpack yarn.lock file bc it is no longer needed, it was reintroduced by accident during a rebase in this branch * React 16.6 snapshot updates, round 2 (mostly Fragment snapshot diffs) * Updated last round of React 16.6 snapshots * Fixes query bar issue with 16.4 gDSFP lifecycle * Updated yarn lock (arraybuffer.slice updated) * Updates snapshots where executeQueryOptions prop appears

jasonrhodes added chore WIP Work in progress labels Oct 28, 2018

markov00 mentioned this pull request Oct 29, 2018

Upgrade react and react-dom to 16.5.2 #23437

Closed

weltenwort mentioned this pull request Nov 28, 2018

[Infra UI] Replace redux source slice with constate container #26121

Merged

jasonrhodes force-pushed the react-16.6 branch from 96b6dee to 0eb1d50 Compare January 3, 2019 16:57

jasonrhodes requested review from a team as code owners January 3, 2019 20:47

legrego reviewed Jan 3, 2019

View reviewed changes

jasonrhodes added 4 commits January 18, 2019 17:50

Updated fragment-based snapshots for x-pack unit tests

ba7c1ab

Removed xpack yarn.lock file bc it is no longer needed, it was reintr…

fb0b4d6

…oduced by accident during a rebase in this branch

React 16.6 snapshot updates, round 2 (mostly Fragment snapshot diffs)

7deb7da

Updated last round of React 16.6 snapshots

a5bfbcc

jasonrhodes force-pushed the react-16.6 branch from 4356fd3 to a5bfbcc Compare January 18, 2019 22:54

chandlerprall mentioned this pull request Jan 22, 2019

Initial go at localizing some components elastic/eui#1450

Merged

Fixes query bar issue with 16.4 gDSFP lifecycle

4893cba

Updated yarn lock (arraybuffer.slice updated)

c83b12d

Updates snapshots where executeQueryOptions prop appears

0bf729e

jasonrhodes force-pushed the react-16.6 branch from 8d41dbb to 0bf729e Compare January 23, 2019 16:38

chandlerprall approved these changes Jan 23, 2019

View reviewed changes

Bargs approved these changes Jan 23, 2019

View reviewed changes

jasonrhodes removed the WIP Work in progress label Jan 23, 2019

legrego approved these changes Jan 23, 2019

View reviewed changes

ogupte approved these changes Jan 23, 2019

View reviewed changes

jasonrhodes merged commit a11e471 into elastic:master Jan 23, 2019

jasonrhodes deleted the react-16.6 branch January 23, 2019 18:46

jasonrhodes mentioned this pull request Jan 23, 2019

[6.x] Updates React et al to 16.6 (#24707) #29203

Merged

chandlerprall mentioned this pull request Jan 23, 2019

Re-add I18n components elastic/eui#1466

Merged

1 task

Updates react to 16.6 (latest) #24707

Updates react to 16.6 (latest) #24707

Conversation

jasonrhodes commented Oct 28, 2018 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Oct 28, 2018

💔 Build Failed

jasonrhodes commented Oct 28, 2018

elasticmachine commented Oct 28, 2018

💔 Build Failed

markov00 commented Oct 29, 2018

sorenlouv commented Oct 29, 2018

jasonrhodes commented Oct 30, 2018

spalger commented Nov 3, 2018 • edited Loading

spalger commented Nov 29, 2018

elasticmachine commented Nov 29, 2018

💔 Build Failed

jasonrhodes commented Nov 29, 2018

jasonrhodes commented Jan 3, 2019

elasticmachine commented Jan 3, 2019

💔 Build Failed

jasonrhodes commented Jan 3, 2019

jasonrhodes commented Jan 3, 2019

legrego commented Jan 3, 2019

legrego Jan 3, 2019

Choose a reason for hiding this comment

jasonrhodes Jan 4, 2019

Choose a reason for hiding this comment

chandlerprall Jan 4, 2019 • edited Loading

Choose a reason for hiding this comment

chandlerprall Jan 4, 2019

Choose a reason for hiding this comment

elasticmachine commented Jan 3, 2019

💔 Build Failed

sorenlouv commented Jan 5, 2019 • edited Loading

joshdover commented Jan 7, 2019

jasonrhodes commented Jan 9, 2019

joshdover commented Jan 9, 2019

jasonrhodes commented Jan 9, 2019

joshdover commented Jan 9, 2019 • edited Loading

elasticmachine commented Jan 18, 2019

💔 Build Failed

jasonrhodes commented Jan 22, 2019

elasticmachine commented Jan 22, 2019

💔 Build Failed

elasticmachine commented Jan 23, 2019

💔 Build Failed

elasticmachine commented Jan 23, 2019

💔 Build Failed

chandlerprall commented Jan 23, 2019

chandlerprall left a comment

Choose a reason for hiding this comment

Bargs left a comment

Choose a reason for hiding this comment

elasticmachine commented Jan 23, 2019

💚 Build Succeeded

jasonrhodes commented Jan 23, 2019

legrego left a comment

Choose a reason for hiding this comment

jasonrhodes commented Jan 23, 2019

jasonrhodes commented Oct 28, 2018 •

edited

Loading

spalger commented Nov 3, 2018 •

edited

Loading

chandlerprall Jan 4, 2019 •

edited

Loading

sorenlouv commented Jan 5, 2019 •

edited

Loading

joshdover commented Jan 9, 2019 •

edited

Loading