[renderHook] Passing rerender props to the wrapper

[udeyrishi]

Describe the Feature

I noticed this change in the legacy react-hooks-testing-library version of renderHook, whereby the props sent to the rerender function were also sent to any provided custom wrapper. However, I'm noticing that the @testing-library/react-native version of the renderHook util does not have this feature. Not sure if this is a bug or a feature request, so decided to just log this to get the conversation going.

Possible Implementations

Similar to testing-library/react-hooks-testing-library#381

Related Issues

testing-library/react-hooks-testing-library#322

[pierrezimmermannbam]

Hello @udeyrishi ! I don't really understand the implementation, passing hook props to the wrapper does not make sense to me, it feels a bit hacky. Maybe we could add another option for wrapper props, I think it would make more sense. That being said I'm not really convinced this feature is needed, do you have a use case for it? I wonder if for such scenarios you could consider writing a test on a component instead

[udeyrishi]

I don't really understand the implementation, passing hook props to the wrapper does not make sense to me, it feels a bit hacky. Maybe we could add another option for wrapper props, I think it would make more sense.

I agree that that API choice would be cleaner. I was just referring to the existing/related implementation, since this felt like a functionality regression. If I were to start from scratch, then yeah I agree that what you suggested is a better choice.

do you have a use case for it?

My use case is to do some assertions on a hook's result, based on a re-render triggered by a wrapping context's value. It's not far off from the example shared in testing-library/react-hooks-testing-library#322. I've tried simplifying my specific use-case by stripping out some business details:

// Is there a cleaner way to accomplish this?

type WrapperPropValue = {
  foo: number
}

const setup = (initialWrapperPropValue: WrapperPropValue) => {
  let updateWrapperPropValue:
    | ((_: WrapperPropValue) => void)
    | undefined;

  const Wrapper: FC<PropsWithChildren> = ({ children }) => {
    const [wrapperPropValue, setWrapperPropValue] = useState(initialWrapperPropValue);

    useEffect(() => {
      updateWrapperPropValue = setWrapperPropValue;

      return () => {
        updateWrapperPropValue = undefined;
      };
    }, []);

    return <SomeHookWrapper somePropThatNeedsToChange={wrapperPropValue}>{children}</SomeHookWrapper>;
  };

  const safeUpdateWrapperPropValue = (
    updatedWrapperPropValue: WrapperPropValue,
  ) => {
    void act(() => {
      updateWrapperPropValue?.(updatedWrapperPropValue);
    });
  };

  const utils = renderHook(useSomeSubjectHook, {
    wrapper: Wrapper,
  });

  return {
    ...utils,
    safeUpdateWrapperPropValue,
  };
};

it('do some assertions after context change', () => {
  const { result, safeUpdateWrapperPropValue } = setup({foo: 1})
  // do assertions on result
  safeUpdateWrapperPropValue({foo : 2})
  // do assertions on updated result
})

[pierrezimmermannbam]

It's kind of hard to tell without knowing what the context or the hook do but ideally you'd want to write a test from a users's perspective so trigger an update of the provider's value through a user interaction and do assertions on something visible by the user. I don't know how hard it is to do that in your case though but that would be a more robust test as it would not depend on implementation details and maybe it would be simpler than what you currently have. If you can't easily trigger an update on the provider you could write a test component that would be you wrapper and the component that uses the hook and then rerender that component, it would look like this:

const TestComponent = ({providerValue}: {providerValue: ContextValue }) => {

 return (<ContextProvider value={providerValue}>
              <ComponentThatUsesTheHook />
             </ContextProvider value={providerValue}>);
}

test('name of the test', () => {
   render(<TestComponent value={initialValue} />);

  // do some assertions

  screen.rerender(<TestComponent value={newValue} />);

  // assert hook did what it was supposed to do
});

[udeyrishi]

but ideally ... what you currently have.

I understand that intention behind that recommendation, and completely agree with it. However, in my specific use case, I'm trying to write a unit test for 1 specific reusable hook, and verify that it behaves as expected as long as the other connected units do their job. It's not intended to be an integration test.

... it would look like this

Yeah that's what I'm doing in the example that I shared above as well--in essence at least. The difference is that in the version that you shared, we'll need to define a ComponentThatUsesTheHook component that simply wraps around the subject hook, and then figure out a way to expose the return value of the hook to the asserter in the test body. That's the "code ugliness" that I (and I assume the author of testing-library/react-hooks-testing-library#322) was hoping to avoid. I can get the job done using my/yours shared sample, it's just not the most elegant of ways 😅

[pierrezimmermannbam]

I understand your point, it's not very elegant indeed. We might add support for the wrapperProps option, it depends on how complicated it is to implement. I assume it would mostly be typescript complexity but we could give it a shot. @mdjastrzebski what do you think of implementing that feature? If we do, @udeyrishi would you you like to contribute?

[mdjastrzebski]

@udeyrishi could you briefly describe the change you are proposing in terms of API so that I can understand it better. I got confused by the idea of passing rerender's newProps which, as far as I understand, are intended for the hook callback to the wrapper component. Do I understand your idea correctly?

To give you context about our implementation of renderHook, as React Hooks Testing Library got deprecated with React 18 release, we followed suite of React Testing Library which implemented a slimmed down renderHook API. Our implementation is basically the same as RTLs one.

[udeyrishi]

@mdjastrzebski All I need is for some way to send initial and updated props to the wrapper as part of the renderHook and rerender calls, respectively. If I were writing this from scratch, then I'd do something like what @pierrezimmermannbam suggested above:

Maybe we could add another option for wrapper props, I think it would make more sense.

I'd imagine it'd look something like this:

test('some test', () => {
  const { rerender } = renderHook(useMyHook, {
    initialProps: { propPassedToHook: 'foo' },
    wrapper: ({ children, /** API addition **/ someWrapperProp }) => <SomeWrapper someWrapperProp={someWrapperProp}>{children}</SomeWrapper>,
    /** API addition **/  initialWrapperProps: { someWrapperProp: 'bar' }
  });

  rerender(
    { propPassedToHook: 'foo-updated' },
    /** API addition **/  { someWrapperProp: 'bar-updated' },
  );
});

This example has these new API additions that don't exist today:

• The wrapper is receiving other props (someWrapperProp in the example), besides just children
• There's a way to specify the initial value of someWrapperProp via initialWrapperProps
• The wrapper props can be updated via a rerender call

That being said, I was simply pointing to this ticket as a reference, because the same problem was previously solved via a different API approach (which is not as explicit as the one I'm proposing above IMHO). That's an alternative that I'd be happy with as well. I assume that behaviour was removed as part of the "slimming down" process.

[mdjastrzebski]

Indeed it seems that RHTL has this "feature" that hook props would be passed to wrapper as props as well: https://github.com/testing-library/react-hooks-testing-library/blob/1e01273374af4e48a0feb1f2233bf6c76d742167/src/helpers/createTestHarness.tsx#L30

TBH I find this quite strange to mix hook and wrapper props as they are not the same thing.

@udeyrishi If you want my personal opinion, I think we should keep the current RTL-based implementation. That implementation seems to be ironed out in a lengthy discussion in their repo.

One of our goals in RNTL is to stay compatible with RTL when possible and deviate only when we are compelled by significant value of such decision. By re-using large parts of their code we save time and can focus our modest resources on high impact initiatives like User Event or semantic (a11y) queries.

Since this seems to be a rather niche feature, I think the complication to the codebase and API surface is not the right tradeoff here.

Could I suggest you check following pattern presented in that discussion:

// Before (RHTL)
const { result, rerender } = renderHook(
  ({a, b}) => useFoo({a, b}),
  {
    initialProps: { a, b, x, y },
    wrapper: ({ a, b, x, y, children }) =>
      <Wrapper a={a} b={b} x={x} y={y}>{children}</Wrapper>,
  }
);

...
rerender({ a: a2, b: b2, x: x2, y: y2 });

// After (RTL)
let a, b, x, y;
const { result, rerender } = renderHook(
  ({a, b}) => useFoo({a, b}),
  {
    initialProps: { a, b },
    wrapper: ({ children }) =>
      <Wrapper a={a} b={b} x={x} y={y}>{children}</Wrapper>,
  }
);

...
a = a2; b = b2; x = x2; y = y2;
rerender({ a: a2, b: b2 });

Let me know how that works for you.

[udeyrishi]

Fair enough, I understand the desire for RNTL to stay close to RTL to save on maintainence cost. I haven't read that linked discussion fully to understand why this "feature" was dropped, or whether it was intentional vs. accidental; regardless, I think my request would probably need to originate on the RTL side then. This is a niche request indeed, and there are workarounds (3 discussed in this ticket) that let me accomplish the goal for now, so this is a task for some other time.

I appreciate your responsiveness, and grace to entertain this random dev on the internet 🙏🏼