feat: Add renderHook by eps1lon · Pull Request #991 · testing-library/react-testing-library (original) (raw)

Firstly, renderHook (or testHook as it was then) was originally part of RTL, but it was decided it didn't belong here and we took over your API and it was removed. I'm curious what triggered this proposal to bring it back in. I get the feeling you disagree with the direction I (I usually say "we" in situations like this, but in this case I wont lump @joshuaellis in with all of the decision I've made over the years) have taken it?

Secondly, I gather from your comments above that you're proposing this renderHook will replace the need for @testing-library/react-hooks all together and not just be a basic version for the majority use case and entry point into out library for more advanced cases? Please correct me if I'm wrong on this.

I'll approach this with the intent I hope it was raised from, which is providing the best tools for the users and try to address some of the points raised above and highlight any "lessons learned" from my time at the helm.

waitForNextUpdate will have to re-implement all of React's heuristics for determining what is going to be batched, which type of effects are being worked on when etc. I don't see its value in our testing philosophy. It may make sense in Enzyme-style testing though.

Yeah, I'm definitely not a fan of waitForNextUpdate. That feels like too much of an implementation detail to me 😬

I think you are giving waitForNextUpdate more credit than it deserves. My mental model for it is essentially a thin layer over waitFor that looks something like:

const currentResult = result.current; await waitFor(() => { expect(result.current).not.toBe(currentResult); });

For transparency, the implementation is not quite this, but due to the nature of hooks, it does function this way for the majority of custom hooks (they frequently return new object, array or function references).

Perhaps it's my lack of understanding about what is to come in React 18, but I don't see why this would need to reimplement any batching logic or is Enzyme-esque in nature. In fact, the only compatibility issue we've had with react 18 so far seems to be that async act is not a thing anymore (or is a thing but is changing somehow), which is not something I think we could have reasonably planned on.

While I can see the concern about implementation details, I do feel like the intent of waitForNextUpdate is more like "I don't care how or when or what made it update, just wait for it to happen". In fact, it cares very little about any of the hooks implementation to do what it does. I do understand that the bigger concern is how people use it rather than specifically how it is implemented though.

I will concede that I personally find waitForValueToChange and waitFor to be more useful than waitForNextUpdate as they generally show better intent in the test code and less likely to fail when changes to the hook code are made that introduce new state stages (although I don't think this actually happens very often for most custom hooks). I have toyed with the idea of deprecating it and removing it, but the maintenance overhead is low and it caters to many peoples basic need, which is "I want to wait for the promise in my useEffect to resolve" (remember, the promise is often not returned or exposed in any meaningful way for the test to use without waiting for the component state to be updated). If it was removed, would you still feel this way?

The other main use case was for hooks using suspense. Again, they could use waitFor or waitForValueToChange instead but it has been convenient for them to use waitForNextUpdate to just wait until the first full render has completed. Suspense is not something that RTL handles out of the box (as far as I'm aware), so this is a case where users will need to handle this themselves in their test code with a custom wrapper.

Regarding duplication: The goal is to reduce duplication. Right now RHTL needs to copy some testing utility from RTL and replay even more React implementation details if they want to be compatible with React 18.

One thing about our async utils that you wont get from using your waitFor (and the reason we don't use dom-testing-library as our base utility) is it wont perform any checks if the result changes between interval checks because there is no mutation to the DOM (if my understanding of why the MutationObserver is being used in dom-testing-library). In our utils we trigger the checks any time the hook value is updated so that our users don't have to set the timing their interval or timeout settings appropriately to not miss their target state (as I view that as an implementation detail). I'm not sure how you intend to overcome that limitation (if you plan to at all).

So my take is that we haven't duplicated the utilities, but rather been inspired by them to write our own for our use case in the same way react-native-testing-library has written their own version, because the dom-testing-library version does not fair well when, shockingly, there is no DOM involved.

I actually wonder if perhaps there isn't space for a common async utils library that we could all collaborate on that has a more generic "something updated please check between intervals" implementation that dom-testing-library (and all it's offspring) can wrap with MutationObserver and other non-DOM based libraries can wrap with our own implementations.

I think one of my major hesitations of this merge, is the loss for native support.

This is a concern for me too. The truth is that that the vast majority of custom hooks are not tied to react-dom or to the DOM at all.

I feel like unless you can get react-native-testing-library onboard and take on the maintenance burden of renderHook as well, you are hanging a large chunk of users out to dry or forcing them into a DOM setup they don't want (or even potentially introduce subtle bugs for them because their test environment no longer matches their prod environment).

It just doesn't fit in the testing-library org. We have a library for different renderers. But then we also have a library for a specific implementation detail that works for multiple renderers?

I disagree. RTL tests React components for web. RNTL tests React components for native. RHTL tests hooks for React Components. As I said above, hooks are not implicitly for DOM or Native (or whatever) components, especially in the library space were they might be expected to work anywhere.

In fact, we moved to a "BYO renderer" model purely for convenience for our users that already had a perfectly good React renderer (react-dom) installed so they didn't have to install another (react-test-renderer) just for us.

But I don't see why we need SSR for hooks but not components.

This bit confuses me. Your API has an option for hydration which I cant think of any other reason to have except for testing some kind of SSR functionality.

The fact of the matter is that hooks behave differently on the server to how they do in the browser and where there is a difference in behaviour people are going to want to test that it behaves as they expect. If I was writing a hook in my library that I advertised as being SSR compatible I'd want to make sure I could back that claim with a test.

But this should happen in another package that doesn't advertise itself as "test like your software is used"

There's just too much inconsistent behavior enabled by the current setup.

I think it's important to remember that our main target audience is library authors (and I consider common utilities in a project to be library-lite code), so the "software" being used is the custom hooks and not the react app using it. As such, there are inherent inconsistencies both with how people think about their test strategy and in how they use the software because they are different. Unless you are suggesting that only libraries that test user interactions are valid testing-library libraries, in which case I'd point out we were invited to be here, so at least at some point that was not a requirement.

I've gone over this with you before and to a lesser extent with Kent, but for the benefit of this discussion I'll try to reiterate in context:

Hooks are not components and component are not hooks (I hope we can all agree on this). Therefore, to suggest that the utilities you would use to test them must be completely consistent doesn't ring true to me (this might be where we start to disagree). Components are visual, interactive and user facing, while hooks are data focussed and code facing.

In fact, I would go so far as to say that when testing a custom hook directly, the fact that is must be called within a component is an implementation detail we want to protect our users from, just like when you are testing a component directly, the fact that it is calling a hook is an implementation detail you want to protect your users from. After all we want to "test like your software is used" and our "software" is a function, not a component or an app.

For example, there is not a great global object, i.e. the DOM, we can run assertions against so utilities like screen and fireEvent don't make much sense for hooks. Instead the values are generally returned from the hooks and are much more localised to the individual calls to the hook. This is also why we return the async utils from renderHook instead of from the import of the library, so they can be aware of when things change to rerun the checks.

Another interesting example is errors. When a component throws an error the expectation is that it bubbles up to the nearest error boundary. It would be reasonable for you to expect your users to use the wrapper to add an error boundary if they wanted to test an error case. For us this isn't so simple. The expectation is that the error is thrown from the function. This is fine when the hook throws the error directly when rendering, but what if it throws in useEffect? This is only caught in error boundaries. What if an async effect sets some state that throws in the subsequent render? This does go to the error boundary as well if there is one, but if not, the render call doesn't throw - it's already passed that point. waitFor wont reject, it doesn't care about renders. This is why we introduced result.error and made result.current throw if result.error is set. It's not ideal but we added it to avoid users having to know exactly how and when the errors are being thrown in order to catch them effectively in their tests. I've speculated that result.error could be removed by throwing from renderHook/rerender and rejecting the async utils, but the techniques used to catch all the ways hooks can error would likely need to remain.

Basically what I'm saying is that by not supporting hooks as hooks, but just as extensions of components, you either leave a lot for users to implement as a wrapper or have to concern themselves with unnecessary implementation details. At this point I wonder if there is much point in having renderHook over just having a docs page on testing hooks that gives them the basic TestComponent implementation to copy/paste and modify. At least then they are far more aware of the component in the mix.

Phew, that was a lot... I hope I didn't come across too defensive. I must admit, reading the comments above I definitely feel a little attacked as if somehow we're not doing our best to align ourselves to the ethos of testing-library. We've put a lot of energy into trying to ensure we are focussed solely on inputs and outputs of the hooks and to test them as close to regular functions as we can. We're always the first to steer people away from RHTL and towards RTL when we think they are not using it for the right use cases.

I want to be clear though, I'm not completely against the idea of renderHook moving into the respective renderer libraries and, in all honesty, not having the maintenance and support burden any more would be somewhat of a relief.

Now some other thoughts...

I have long thought of intialProps to be a bit of a mistake and we've only kept it around for backwards compatibility. Having the callback accept no props and have variables for them be updated separately to rerender calls is not a huge overhead for the users and is easier for many to understand what is going on (less invisible lines between args and values), e.g.

const { result, rerender } = renderHook((message) => useSomeHook(message), { initialProps: 'hello' });

rerender('world');

vs.

let message = 'hello'; const { result, rerender } = renderHook(() => useSomeHook(message));

message = 'world'; rerender();

The second example is clearer to me that message will change when rerender is called.

I'll also add that I very rarely see rerender and unmount used in the wild. Most tests call renderHook and either use a returned function to trigger a stage change or are using the async utils to wait for a state change to happen before asserting the results.

Similarly, result.all was a mistake. I was on the fence about it, but eventually added it as it provided some pseudo SSR functionality before we had proper SSR support. Now I mostly see people abusing it. I haven't removed it yet because I do think there is an argument that the returned values between renders can be important in the context of hooks as, while they are not observable to users, they are observed by the components consuming them and could cause an error to be thrown if a bad value is returned. In practice though, I think this is rare and I doubt anyone is actually testing for it anyway.

I find your use of useEffect to set result.current interesting and I'm wondering if we should be doing that ourselves now. What is the benefit of that over just setting it in the render function? Is it possible that the final rendered value is not the same as the one that makes it to useEffect?

Finally, users destructuring result.current (i.e. const { result: current } = renderHook(...) or const [value, setValue] = result.current) are, easily, the biggest cause of bug reports and support requests. If we are going to introduce a new renderHook, perhaps it's a good time for some iteration on the API to see if we can't reduce the cognitive overload the ref-like (or actual ref in you case) style seems to cause so many people.

That is all for now. I hope this is all makes some sense from outside my head. I'm happy to clarify or dive deeper on any of it if you want.