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 to React 17 #3388

Merged
merged 2 commits into from
Sep 10, 2021
Merged

Update to React 17 #3388

merged 2 commits into from
Sep 10, 2021

Conversation

antgamdia
Copy link
Contributor

Description of the change

This PR simply upgrades the react version from 16 to 17, which is mostly an internal refactor with no new features (they will add them in React 18 instead).
In fact, they say:

We’ve only had to change fewer than twenty components out of 100,000+ in the Facebook product code to work with these changes, so we expect that most apps can upgrade to React 17 without too much trouble. Please tell us if you run into problems.

However, the enzyme adaptor we are using for the tests has to be updated accordingly (see drawbacks).

Benefits

We will be ready for the upcoming react versions.

Possible drawbacks

There is no official enzyme adapter yet (although there is an open PR enzymejs/enzyme#2430). In the meanwhile, there is an unofficial module published that is working (working=our tests are passing, since enzyme is just for testing) (more info enzymejs/enzyme#2429 (comment))

Applicable issues

N/A

Additional information

There is no reason to merge this PR now, as it is just an update I wanted to perform some time ago, but, as I said, no new features so far. We can hold it until we release 2.4.1, for instance. Up to you, I don't foresee any errors, but let's see what our beloved CI thinks.

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Copy link
Contributor

@absoludity absoludity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI is happy. I'm happy :)


mount(<TestComponent />);
expect(Object.keys(listeners).length).toBe(1);
expect(Object.keys(listeners).length).toBe(2);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not 100% sure, but it has something to do with the changes in the event delegation [1], but I just manually confirmed the hook was working as expected (that is, clicking outside the context switcher and right menu is working as expected), but I performed this change for the tests to pass.

[1] https://reactjs.org/blog/2020/10/20/react-v17.html#changes-to-event-delegation

@antgamdia antgamdia merged commit 3bb99b1 into vmware-tanzu:master Sep 10, 2021
@antgamdia antgamdia deleted the react17 branch September 10, 2021 09:41
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.

2 participants