Skip to content

test: Add React 18 to test matrix #771

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Feb 16, 2022
Merged

test: Add React 18 to test matrix #771

merged 7 commits into from
Feb 16, 2022

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Nov 23, 2021 •
edited
Loading

No description provided.

@eps1lon eps1lon changed the title Test/react 18 Test with React 18 Nov 23, 2021
eps1lon
eps1lon commented Nov 23, 2021
View reviewed changes
test/TransitionGroup-test.js Outdated
act(() => {
jest.runAllTimers();
});
expect(log).toEqual(['appear', 'appear', 'appearing', 'appeared']);
Copy link
Member Author

Choose a reason for hiding this comment

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

The second appear is new i.e. there is an additional onEnter call.

eps1lon
eps1lon commented Nov 23, 2021
View reviewed changes
test/TransitionGroup-test.js Outdated
act(() => {
jest.runAllTimers();
});
expect(log).toEqual(['enter', 'enter', 'entering', 'entered']);
Copy link
Member Author

Choose a reason for hiding this comment

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

The second enter is new i.e. there is an additional onEnter call.

@koba04 koba04 mentioned this pull request Nov 29, 2021
Closed
1 task
@koba04
Copy link
Member

koba04 commented Dec 13, 2021 •
edited
Loading

@eps1lon
React v18 RC has been released, so it's the time to test with React v18. Could you make this PR ready for review?

@eps1lon
Copy link
Member Author

eps1lon commented Dec 13, 2021

@eps1lon React v18 has been released, so it's the time to test with React v18. Could you make this PR ready for review?

React 18 was not released yet. Only a release candidate is available. I've updated the branch but it shouldn't be merged yet considering it's only using a release candidate.

@koba04
Copy link
Member

koba04 commented Dec 13, 2021

@eps1lon

React 18 was not released yet.
Yes, it was a typo.

I've updated the branch but it shouldn't be merged yet considering it's only using a release candidate.

Yes, we don't drop support for older versions of React, so we shouldn't update React in package.json.
At this time, we should run tests with React v16, v17, and v18 RC, so it would be nice if we could add React v18 in the env and pass all tests with all versions.
Is it possible?

@eps1lon
Copy link
Member Author

eps1lon commented Dec 13, 2021 •
edited
Loading

Is it possible?

Sure, but we're blocked by #772. I've tried contacting @jquense in the past (via GitHub ping and Twitter DM) but never got a response. Not sure how to reach him.

@koba04
Copy link
Member

koba04 commented Dec 13, 2021

@eps1lon

Sure, but we're blocked by #771. I've tried contacting @jquense in the past (via GitHub ping and Twitter DM) but never got a response. Not sure how to reach him.

Yeah, I'll reach him again.
We can only release new versions by semantic-release on TravisCI, so we can't migrate TravisCI to GitHub Actions without his help.
What about adding React v18 RC as the target of TravisCI?

@jquense
Copy link
Collaborator

jquense commented Dec 13, 2021

I have been summoned. Sorry if i've ignored anyone, it's inadvertent, i've just not have a ton of OSS time or energy lately.

What do y'all need from me? I'm happy to hand permissions or move stuff around as needed. Also from a maintenance standpoint, I'd just drop react 16 support at this point. it's been over a year since 17 was released, make y'alls life easier.

@eps1lon
Copy link
Member Author

eps1lon commented Dec 13, 2021

I have been summoned. Sorry if i've ignored anyone, it's inadvertent, i've just not have a ton of OSS time or energy lately.

Hey,

#772 needs your attention. Specifically, the environment secrets from Travis CI need to be ported to GitHub actions. I think this mainly affects semantic-release i.e. a npm release token.

@jquense
Copy link
Collaborator

jquense commented Dec 13, 2021

I've added @koba04 to the npm package, they should now be able to use their token for any CI setup 👍

@koba04
Copy link
Member

koba04 commented Dec 14, 2021

@jquense
Thank you! Could you add me as an admin of this GitHub repository?
We have a plan to migrate CI to GitHub Actions. That requires setting an npm token to the GitHub repository and it requires an admin privilege of the repository.

I also want to add @eps1lon as a maintainer of this repository.

@koba04
Copy link
Member

koba04 commented Jan 7, 2022

@eps1lon I've invited you as an admin of the GitHub and npm of this package, so we can move this forward👍

@eps1lon eps1lon changed the title Test with React 18 test: Add React 18 to test matrix Jan 17, 2022
@eps1lon eps1lon marked this pull request as ready for review January 17, 2022 20:46
@eps1lon eps1lon requested a review from koba04 January 17, 2022 20:46
koba04
koba04 reviewed Jan 21, 2022
View reviewed changes
Copy link
Member

@koba04 koba04 left a comment

Choose a reason for hiding this comment

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

Thank you for your great works 👍 👍
I've left some comments, so please confirm them🙏

test/SwitchTransition-test.js
Comment on lines +109 to +111
act(() => {
jest.runAllTimers();
});
Copy link
Member

Choose a reason for hiding this comment

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

Why is this call added?

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me check. I think it was due to previous tests relying on implementation details related to React scheduling timings. Will check what specifically was happening.

test/Transition-test.js
@@ -334,16 +356,19 @@ describe('Transition', () => {

onEntered() {
expect(nodeRef.current.textContent).toEqual(`status: ${ENTERED}`);
expect(count).toEqual(2);
done();
Copy link
Member

Choose a reason for hiding this comment

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

Should we have to verify this onEntered has been called?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is similar to #771 (comment):

This condition moved down to

     await waitFor(() => {
        expect(count).toEqual(2);
      });

@eps1lon @koba04
Apply suggestions from code review
8ac97da 
Co-authored-by: Toru Kobayashi <koba0004@gmail.com>
@koba04
Copy link
Member

koba04 commented Feb 15, 2022

I'll approve this when #771 (comment) has been addressed.

@eps1lon
Copy link
Member Author

eps1lon commented Feb 15, 2022

I'll approve this when #771 (comment) has been addressed.

See #771 (comment)

koba04
koba04 approved these changes Feb 16, 2022
View reviewed changes
Copy link
Member

@koba04 koba04 left a comment

Choose a reason for hiding this comment

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

Thank you, LGTM! 👍

@koba04 koba04 merged commit 1424fc8 into reactjs:master Feb 16, 2022
@github-actions
Copy link

🎉 This PR is included in version 4.4.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@koba04 koba04 koba04 approved these changes

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eps1lon @koba04 @jquense