RenderResult/rerender enhancements

[m3fawner]

Describe the feature you'd like:

Enhance the rerender usage to not require the use of a Javascript getter function for properties of result.

It took approximately 45 minutes of digging through code examples, blogs, RHTL code, and a substantial number of console logs to figure out why:

const { result: { all }, rerender } = renderHook(myHook);
rerender();
console.log(all.length); // always 1

As it turns out, a console.log of the entire return value of renderHook was what I needed (I got desperate and was looking for undocumented return values).

    {
      result: { all: [Getter], current: [Getter], error: [Getter] },
      rerender: [Function: rerenderHook],
      unmount: [Function: unmountHook],
      waitFor: [AsyncFunction: waitFor],
      waitForValueToChange: [AsyncFunction: waitForValueToChange],
      waitForNextUpdate: [AsyncFunction: waitForNextUpdate]
    }

The implementation of result relying on getters means I effectively cannot destructure the result, which feels like common practice in the JS community.

Suggested implementation:

It'd be excellent to return the newly generated result from rerender instead of it being void. That way I can do:

const { result: { current: initial }, rerender } = renderHook(myHook);
const { result: { current: subsequent }} = rerender();

Another alternative is to make the all reference a single array reference throughout to preserve the results & allow the destructure to reference the collection. This wouldn't work, however, for current or error.

Teachability, Documentation, Adoption, Migration Strategy:

At a minimum, the documentation for the RenderResult should be updated to make it clear that it should not be destructured given the implementation details.

[joshuaellis]

At a minimum, the documentation for the RenderResult should be updated to make it clear that it should not be destructured given the implementation details.

It's very clearly written here in caps with NOTE, however if you think there's a way to improve the documentation, please feel free to make a contribution via PR, docs can always be made better 😄

I think there's a reason why we've made result a getter... I'm struggling to remember it right now.

[m3fawner]

NOTE: There's a gotcha with updates. renderHook mutates the value of current when updates happen so you cannot destructure its values as the assignment will make a copy locking into the value at that time.

To be clear, this calls out current not the entirety of the result. Some verbiage change there would certainly help (and styling, in my opinion undiscoverable blocks of text aren't useful regardless of how well they're written).

The core of my request is around the functionality more so than the docs. That type of note means this has been a known pain point of consumers.

[mpeyper]

If I'm understanding correctly, the main issue here is that result.all returns a new array each time it's called, right? current and error is inconsequential to the implementation as the values within them would change references between renders (hence the warning about destructuring in the docs).

This implementation was somewhat intentional for 2 reasons:

1. The internal representation of the results is different from what result.all returns so we're mapping in the getter, not just returning an existing array.
2. We wanted to prevent users from being able to do result.all.push('whatever') and mess with our internal state

It would be interesting to see if we can somehow support updating the original array while still maintaining the above constraints.

It'd be excellent to return the newly generated result from rerender instead of it being void

This has been suggested before and a very early alpha implementation of this library actually had a similar API. Technically, it would not be difficult to do this, but the issue comes up in the teachability of it. The majority of issues we see come through here and discord are the result of destructuring result.current. If we extend the API as suggested, that become a legitimate strategy for a subset of use cases, that still breaks in others.

For example, if calling rerender triggers a side effect that ends up asynchronously updating the hook state, you're in a position where neither initial or subsequent hold the current result. While that might be fine for this particular test case, and you may be comfortable enough to adapt the next test to not use destructuring or the rerender result, we need to ensure that the tools is approachable to beginners without being confusing or leading them down a path of failure.

[m3fawner]

Thank you for the justification & explanation, @mpeyper . I understand I'm a small subsection of a larger audience and my views are going to be highly biased here, so with that...

To me, rerender being void & causing a caveat like "you can't destructure the result", is more problematic than adding documentation to rerender itself. The destructure comment is nestled under "updates" in "Usage" but is really in reference to the internals of RenderHookResult, where this behavior is not documented.

Again, referencing my own bias, it feels easier to return a value in rerender and explain it there that it is synchronous at that point.

Another thought tangential to all of this is a way to get the result whenever we want it, directly from the internals of the library. I think a good portion of this conversation would be resolved by being able to access the result on demand and let your users managing the timing of when to evaluate the getters.

[mpeyper]

Sorry @m3fawner, this fell off my radar.

Another thought tangential to all of this is a way to get the result whenever we want it, directly from the internals of the library. I think a good portion of this conversation would be resolved by being able to access the result on demand and let your users managing the timing of when to evaluate the getters.

Don't we have this already with result.current? Any time you reference it you will get the current value for the hook result, right?

const { result, rerender, waitForNextUpdate } = renderHook(myHook);

const initial = result.current;

const { result: { current: subsequent }} = rerender();

const subsequent = result.current;

subsequent.triggerAsyncThing();

const subsequent2 = result.current;

await waitForNextUpdate();

const finalValue = result.current;

Or have I completely missed your point?

[m3fawner]

this fell off my radar.

No worries, open source is tough, and we're all busy folks :)

Your example highlights the problem, but you've adjusted the code to work with the knowledge you and I have, which is that the result.current statement is actually a getter.

My expectation, before knowing that detail, was I could destructure the renderHook statement on your line 1 to access all, and it would be an immutable reference (say, an array that is pushed to). That way the behavior would look something more like this:

const { result: { all }, rerender } = renderHook(myHook);
rerender();
expect(all[1]).toBe(something);

my recommended solution, which is currently not available, would be to return the same result object structure from the re-render, which would require less of a re-work in terms of the underlying code.

That way, I could do, as your example is more aligned with:

const { result: { current }, rerender } = renderHook(myHook);
const { result: { current: subsequent } } = rerender();

However, rerender is currently void.

Ultimately, my ask is to not have to be aware of the internals of the code (getter usage) in order to work with the library. This is ultimately stylistic, as there are clearly ways around it, but it's also something that threw me for a loop until I happened to get the right console log highlighting the getter nature.

While @joshuaellis was able to point me to documentation, it ultimately was not very discoverable for me.

The way I see it is there's three possible paths:

1. Modify the use of getters for RenderHookResult which is probably an obscene ask
2. Make rerender return its RenderHookResult
3. Update the docs to be structured more naturally (the note highlighted is under Usage -> Basic Hooks -> Updates and is a small note at the end of that section). Perhaps pointing this out in the renderHook Result documentation in reference/API?

These are largely just suggestions, I ultimately have a work around, as we've discussed above, but it would make it easier to pick up the library with one fewer gotcha.

Thanks for taking the time!