-
-
Notifications
You must be signed in to change notification settings - Fork 632
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
chore(deps): update dev dependencies #2830
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
Size Change: +4 B (0%) Total Size: 90.8 kB
ℹ️ View Unchanged
|
Preview in LiveCodesLatest commit: d7c46c2
See documentations for usage instructions. |
"react": "19.0.0-rc.0", | ||
"react-dom": "19.0.0-rc.0", | ||
"rollup": "^4.25.0", | ||
"react": "19.0.0-rc.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updating react to rc.1 makes tests to fail.
@kretajak I wonder if you are familiar with this case. Do we misuse RTL for async tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will take a look at that. The way RTL is used here to test async stuff is fine from my perspective.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like broken tests are using Suspense
and Suspense
behavior was changed in RC1 (facebook/react#29898 (comment)).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking a look. I know the Suspense behavior was changed, but didn't consider if it really broke it.
https://stackblitz.com/edit/vitejs-vite-hbi7w9?file=src%2FApp.tsx&terminal=dev This looks like working. So, it's probably test only issue.
- 0.0.0-experimental-5c56b873-20241107 | ||
- 19.0.0-rc.1 | ||
- 19.0.0-rc-e1ef8c95-20241115 | ||
- 0.0.0-experimental-e1ef8c95-20241115 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the list of commits for the change:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More precisely, it's between these commit that cause the error:
facebook/react@380f5d6...b01722d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, enableSiblingPrerendering
should only be the meaningful change.
tests/react/basic.test.tsx
Outdated
@@ -34,6 +36,28 @@ const useCommitCount = () => { | |||
return commitCountRef.current | |||
} | |||
|
|||
/* eslint-disable testing-library/no-unnecessary-act */ | |||
const renderRoot = async (element: ReactNode) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only hack I came up with. If it looks good, I'll work on other files too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's interesting why it works, especially as render
from RTL and userEvent
methods are already wrapped with act
. Kent C Dodds mentions it as bad practice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like we should await all act()
. render
from RTL throws away the promise from act()
call.
https://github.com/testing-library/react-testing-library/blob/main/src/pure.js#L145
This workaround isn't ideal because we can't test the initial "Loading...".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's very tricky, but for now, it seems to pass the CI.
wrapper: WrapperComponent | ||
}) { | ||
- (0, _actCompat.default)(() => { | ||
+ await (0, _actCompat.default)(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As it turns out, this helps.
const userEvent = { | ||
// eslint-disable-next-line testing-library/no-unnecessary-act | ||
click: (element: Element) => act(() => userEventOrig.click(element)), | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hoped to patch user-event
too, but didn't find an easy way.
for v2.10.3
with patching RTL for react 19-rc.1.