-
Notifications
You must be signed in to change notification settings - Fork 47.3k
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
Add ref callback test for cleanup fn vs null call #28895
Conversation
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.
would it be worth calling root.unmount()
and making assertions on the behavior around that as well?
// New ref is setup | ||
expect(setup2).toBeCalledWith('test-div2'); | ||
expect(setup2).toHaveBeenCalledTimes(1); | ||
expect(cleanUp2).toHaveBeenCalledTimes(0); |
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.
cleanUp2
doesn't seem to be used anywhere?
Comparing: 699d03c...2ab758f Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
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.
oh... i see there's already another test covering what i suggested.
thanks!
Used this test scenario to clarify how callback refs work when detached based on the availability of a cleanup function to update documentation in reactjs/react.dev#6770
Checking it in for additional test coverage and test-based documentation