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

Switch to IS_REACT_ACT_ENVIRONMENT instead of act when needed when using react 18 #1131

Merged
merged 5 commits into from Sep 22, 2022

Conversation

AugustinLF
Copy link
Collaborator

Closes #1093. Implementation is copied from testing-library/react-testing-library#1031

Without the react 17/18 branches (i.e only using the new API), tests pass but are full of console.error related to act and async act. With his new implementation, everything passes (including the new tests that used to fail on react 18), without calls to console.error.

src/index.ts Outdated Show resolved Hide resolved
src/act.ts Outdated Show resolved Hide resolved
src/act.ts Outdated Show resolved Hide resolved
src/act.ts Outdated Show resolved Hide resolved
src/act.ts Outdated Show resolved Hide resolved
src/act.ts Outdated Show resolved Hide resolved
Copy link
Member

@mdjastrzebski mdjastrzebski left a comment

Choose a reason for hiding this comment

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

@AugustinLF big WOW for creating this PR 🚀

I've done some cursory review and left few comments, I plan to dig a bit deeper into that, as the PR is quite complex.

@AugustinLF
Copy link
Collaborator Author

@mdjastrzebski I've updated the PR with your feedback, let me know if you need anything else :)

Copy link
Member

@mdjastrzebski mdjastrzebski left a comment

Choose a reason for hiding this comment

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

Almost there 🚀 , looking forward to merging that :-)

@AugustinLF
Copy link
Collaborator Author

AugustinLF commented Sep 22, 2022

We're good @mdjastrzebski :)

@mdjastrzebski
Copy link
Member

@AugustinLF It seems I've broken build by pushing a simple commit that adds one line for formatting. Can you add some minimal commit so that the build is triggered as your user?

Copy link
Member

@mdjastrzebski mdjastrzebski left a comment

Choose a reason for hiding this comment

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

Looks great! Though I've broke the build by adding an empty line for formatting purpose.

@AugustinLF
Copy link
Collaborator Author

@mdjastrzebski fixed, added an empty commit!

@mdjastrzebski mdjastrzebski merged commit 807898e into callstack:main Sep 22, 2022
@AugustinLF AugustinLF deleted the fix/react-18-act branch September 23, 2022 07:40
@thymikee
Copy link
Member

I wish React team documented this better. This horrific experience for testing library authors.

@AugustinLF
Copy link
Collaborator Author

I wish React team documented this better. This horrific experience for testing library authors.

Yeah, the documentation was there in the WG, but not in the main release notes, so a bit sketchy.

@mdjastrzebski what do you think of creating an RC release of that? I'm currently working on our react 18 upgrade and that'd be some good battle testing.

@mdjastrzebski
Copy link
Member

mdjastrzebski commented Sep 23, 2022

Going through RC sounds good, for extra safety.

From our analysis of both RTL and React 18 code with @thymikee , we came to conclusion that IS_REACT_ACT_ENVIRONMENT option affects only logging of sync act error messages.

In short:
IS_REACT_ACT_ENVIRONMENT === false
=> isReactActEnvironmentGlobal === false`
=> !isLegacyActEnvironment
=> disable sync act warning in React

@AugustinLF
Copy link
Collaborator Author

AugustinLF commented Sep 23, 2022

Yes. But on top of that we updated the implementation of waitFor:

// from
let result: T;
await act(async () => {
  result = await waitForInternal(expectation, optionsWithStackTrace);
});

// to
await waitForInternal(expectation, optionsWithStackTrace);

@mdjastrzebski
Copy link
Member

@AugustinLF we will make the release after the weekend, as I need @thymikee for npm publish access rights.

@mdjastrzebski
Copy link
Member

🎉 This PR is included in version 11.2.0 🎉

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.

Unexpected failing test when mixing promises and useEffect
3 participants