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: correct type inference for props when initialProps is used #1421

Conversation

pierrezimmermannbam
Copy link
Collaborator

Summary

Fixes #1420

Because we were using conditionals in RenderHookOptions and the way Typescript works with conditionals, Props type was inferred too strictly. For instance when doing:

 renderHook(
    (num: number | undefined) => useMyHook(num),
    {
      initialProps: 5,
    }
  );

Props type would be infered as 5 and not number | undefined which would be the expected type.

This PR fixes this issue by removing the use of the conditional. This doesn't change much the behavior and it aligns with the types from testing library react hooks and react testing library

Test plan

I added a test case to check that the type is inferred correctly

src/renderHook.tsx Outdated Show resolved Hide resolved
Copy link
Member

@mdjastrzebski mdjastrzebski left a comment

Choose a reason for hiding this comment

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

@pierrezimmermannbam looks good. I've suggested some minor changes based on RTL's typing.

@codecov
Copy link

codecov bot commented Jun 29, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01 🎉

Comparison is base (8c22290) 96.88% compared to head (da2e542) 96.89%.

❗ Current head da2e542 differs from pull request most recent head c5d1e47. Consider uploading reports for the commit c5d1e47 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1421      +/-   ##
==========================================
+ Coverage   96.88%   96.89%   +0.01%     
==========================================
  Files          65       65              
  Lines        3693     3705      +12     
  Branches      538      539       +1     
==========================================
+ Hits         3578     3590      +12     
  Misses        115      115              
Impacted Files Coverage Δ
src/renderHook.tsx 100.00% <100.00%> (ø)

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@mdjastrzebski mdjastrzebski left a comment

Choose a reason for hiding this comment

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

Looks good, let's merge!

@mdjastrzebski mdjastrzebski merged commit fa0c86e into callstack:main Jul 19, 2023
@pierrezimmermannbam pierrezimmermannbam deleted the fix/renderHookPropsInferedType branch July 19, 2023 11:50
@mdjastrzebski
Copy link
Member

This PR has been released in v12.1.3 🚢

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.

renderHook's rerender not has undefined prop type unless explicitly typed
2 participants