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

fix: useTrackRenders should be usable inside of wrappers #8

Closed
wants to merge 2 commits into from

Conversation

phryneas
Copy link
Member

@phryneas phryneas commented Nov 7, 2024

This swaps the ParentWrapper and Wrapper components in the render function call, so that calls that require context (e.g. useTrackRenders) can also be used in user-supplied wrapper options.

Fixes #7

This swaps the `ParentWrapper` and `Wrapper` components in the `render` function call, so that calls that require context (e.g. `useTrackRenders`) can also be used in user-supplied `wrapper` options.
@phryneas phryneas requested a review from jerelmiller November 7, 2024 09:03
Copy link

pkg-pr-new bot commented Nov 7, 2024

yarn add https://pkg.pr.new/testing-library/react-render-stream-testing-library/@testing-library/[email protected]

commit: 58adfb8

@phryneas
Copy link
Member Author

phryneas commented Nov 7, 2024

@jerelmiller all that said, I want to take a step back: do we really want to do this?

Especially if we add "auto-injection" of useTrackRender into play in the future, this would e.g. capture the ApolloProvider as a "rerendered component" - which might be less than desirable. Also, you might not want to track each rerender of components in Wrapper that doesn't affect the children.

This might actually kinda take a feature away, not fix one.

At this point, my gut feeling is "if you want to test it, it should be part of rendered JSX. Putting something into the wrapper is an indicator that it is important for the test to run, but not part of the test.".

@phryneas phryneas marked this pull request as draft November 7, 2024 09:19
@jerelmiller
Copy link
Collaborator

That's fair. I think I'm stuck thinking in terms of our old implementation, but your reasons for not wanting to go this route make sense.

Copy link
Collaborator

@jerelmiller jerelmiller left a comment

Choose a reason for hiding this comment

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

I trust your judgement on whether you want to actually introduce this or not but here's an approval either way 🙂

@phryneas
Copy link
Member Author

phryneas commented Nov 7, 2024

I think let's close it for now. If we get a lot of feedback in this direction, we can still pick it up, or introduce something like an InnerWrapper option.

@phryneas phryneas closed this Nov 7, 2024
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.

Cannot use useTrackRenders when component is used in the wrapper option
2 participants