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

How to wrap a hook with a Provider with prop passing? #1233

Open
acrowell1-chwy opened this issue Aug 16, 2023 · 13 comments
Open

How to wrap a hook with a Provider with prop passing? #1233

acrowell1-chwy opened this issue Aug 16, 2023 · 13 comments
Labels
question Further information is requested

Comments

@acrowell1-chwy
Copy link

What is your question:

I have seen the following which was helpful: testing-library/react-hooks-testing-library#23
This question steams further from that with regards to passing initialProps.
I have updated React and DOM ("react": "18.2.0", "react-dom": "18.2.0",) and the React Testing Library ("@testing-library/react": "14.0.0",) to the most recent versions. I have followed that pattern, but notice that the props passed into Provider are only available via children. Please see the example below (borrowed from the tests within the library https://github.com/testing-library/react-hooks-testing-library/blob/1e01273374af4e48a0feb1f2233bf6c76d742167/src/__tests__/useContext.test.tsx):

 test('should update value in context when props are updated', () => {
    const TestContext = createContext('foo');

    const wrapper = ({
      current,
      children,
    }: {
      current: string;
      children: any;
    }) => {
      console.log({ current });
      console.log({ children });
      console.log('nested', children.props.renderCallbackProps);

      return (
        <TestContext.Provider value={current}>{children}</TestContext.Provider>
      );
    };

    const { result, rerender } = renderHook(() => useContext(TestContext), {
      wrapper,
      initialProps: {
        current: 'bar',
      },
    });

    rerender({ current: 'baz' });

    expect(result.current).toBe('baz');
  });

// console.logs

console.log
{ current: undefined }

console.log
{
  children: {
    '$$typeof': Symbol(react.element),
    type: [Function: TestComponent],
    key: null,
    ref: null,
    props: { renderCallbackProps: [Object] },
    _owner: null,
    _store: {}
  }
}

console.log
nested { current: 'baz' }

Was this the intended change? Are we supposed to only pass children to a wrapper and then desconstruct those props from it? Can we update the tests or docs to represent the recommended pattern?

@acrowell1-chwy acrowell1-chwy added the question Further information is requested label Aug 16, 2023
@acrowell1-chwy
Copy link
Author

@mpeyper I saw you answer in the previous question. Tagging you here for an extension of it.

@mpeyper
Copy link
Member

mpeyper commented Aug 16, 2023

Hey @acrowell1-chwy, just want to double check that you have stopped using this library (@testing-library/react-hooks and migrated to @testing-library/react), right?

If that’s the case, the wrapper props are handled differently than they were here so our tests and docs do not represent how they intend it to be used. I suggest raising an issue over there instead (or I can move this one if you prefer?).

Unfortunately, documenting the difference of wrapper props behaviour is I got up to in the “never released” migration guide.

@acrowell1-chwy
Copy link
Author

@mpeyper you are correct, I am using renderHook from @testing-library/react. Yes, please move this to an issue. Thank you for confirming!

@mpeyper mpeyper transferred this issue from testing-library/react-hooks-testing-library Aug 16, 2023
@mpeyper
Copy link
Member

mpeyper commented Aug 16, 2023

@mpeyper you are correct, I am using renderHook from @testing-library/react. Yes, please move this to an issue. Thank you for confirming!

Done 🙂

FWIW, I believe their implementation should only pass children to the wrapper and never any props, so you will likely need to manage them manually outside of the wrapper/renderHook callbacks.

@acrowell1-chwy
Copy link
Author

FWIW, I discovered that props passed from initialProps were nested within children -> const { current } = children?.props?.renderCallbackProps;. If that helps anyone.
Appreciate the prompt response and aid. 😸

Code example:

test("should update value in context when props are updated", () => {
  const TestContext = createContext("foo");

  const wrapper = ({ children }: { children: any }) => {
    const { current } = children?.props?.renderCallbackProps;
    return (
      <TestContext.Provider value={current}>{children}</TestContext.Provider>
    );
  };

  const { result, rerender } = renderHook(() => useContext(TestContext), {
    wrapper,
    initialProps: {
      current: "bar",
    },
  });

  rerender({ current: "baz" });

  expect(result.current).toBe("baz");
});

@acrowell1-chwy
Copy link
Author

acrowell1-chwy commented Aug 17, 2023

@mpeyper do you know if this const { current } = children?.props?.renderCallbackProps; is intentional for pulling props from initialProps (as shown above)? Happy to contribute/help.

@mpeyper
Copy link
Member

mpeyper commented Aug 18, 2023

I don’t know offhand and am not really involved with this implementation. My understanding was only the TestComponent was passed to the wrapper to be wrapped. Any props you are reading off the component is likely unsafe and not recommended.

@elboletaire
Copy link

elboletaire commented Aug 21, 2023

This is how it is documented: https://testing-library.com/docs/react-testing-library/api/#renderhook-options

Considering it's not working as documented, this should be tagged as a bug, not as a question. Props should be available alongside the children one.

Anyone trying to test react 18 components following the documentation will face with this problem if they try to add any initialProps. Using the old @testing-library/react-hooks is not an option for anyone using react 18, since it throws errors about a react 18 deprecation in the render method.

Thanks for the workaround @acrowell1-chwy.

Edit: Ok my bad I was reading the old documentation, but the new way to implement it is terrible... having to create a wrapper is not a good solution, because later on when trying to use rerender it simply does not update the props. Using @acrowell1-chwy solution works better for all cases.

@mpeyper
Copy link
Member

mpeyper commented Aug 21, 2023

For other’s reference, the new docs are here https://testing-library.com/docs/react-testing-library/api/

Whatever workarounds you use, please consider that it is not part of the supported API and could change without notice.

@elboletaire
Copy link

For other’s reference, the new docs are here testing-library.com/docs/react-testing-library/api

I guess you answered before my edits 🙃

@elboletaire
Copy link

elboletaire commented Aug 21, 2023

Whatever workarounds you use, please consider that it is not part of the supported API and could change without notice.

Yeah but the main API looks broken... the documented workaround (creating a createWrapper function) does not work for the rerender props update case. Shouldn't this be considered a bug then?

Edit: BTW just to be clear: the old library works well here. Its only issue is being incompatible with react 18. I understand removing some functionalities and methods when porting the library here, but this initialProps part does not seem to be working as intended now.

@mpeyper
Copy link
Member

mpeyper commented Aug 21, 2023

The workaround suggested is highlighting that’s it’s not handling anything for you automatically. You could pass an object to the wrapper then update the values in the object and trigger a rerender if you need them to change, but it’s all going to be manual on your part. It’s not a bug, just frustrating.

I’d just like to reiterate though that I’m not associated with the new implementation at all. You’ll need to wait for someone from @testing-library/react to chime in providing better support for this use case. Note though that I imagine part of the reason it’s less supported now is that the new version is piggy backing off the render function used for testing components and they may not want to bleed the logic that far (not something we had to worry about in @testing-library/react-hooks)

@davidgoli
Copy link

davidgoli commented Dec 19, 2023

Agreed with the comments here, passing the "props" to the hook's render function (instead of the wrapper component) makes rerender much less useful. Perhaps I might recommend adding a second function like 'rerenderWrapper` that works this way? Something like:

const TrackingProvider = (props: { trackingProps: { userId: number }, children: React.ReactNode }) => {
  // .. Context provider that maintains state
}

// useTracking reads state & dispatches tracking events
const { rerenderWrapper } = renderHook(() => useTracking(), {
  wrapper: TrackingProvider,
  initialProps:  {
    trackingProps: { userId: 123 }
  },
})

expect(track).toHaveBeenCalledWith({ userId: 123 })

rerenderWrapper({ trackingProps: { userId: 456 })

expect(track).toHaveBeenCalledWith({ userId: 456 })

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants