fix: add prop to render to skip configureHostComponentNamesIfNeeded

[KrastanD]

Summary

#1423

Test plan

This change can be tested by running the renderHook.test.tsx tests after clearing the jest cache. Before my change, the first test takes 445 ms and after my change it takes 5 ms.

Before:

yarn run v1.22.19
$ ~/Code/react-native-testing-library/node_modules/.bin/jest ./src/__tests__/renderHook.test.tsx
 PASS  src/__tests__/renderHook.test.tsx
  ✓ gives comitted result (445 ms)
  ✓ allows rerendering (1 ms)
  ✓ allows wrapper components (2 ms)

Test Suites: 1 passed, 1 total
Tests:       3 passed, 3 total
Snapshots:   0 total
Time:        1.273 s
Ran all test suites matching /.\/src\/__tests__\/renderHook.test.tsx/i.
✨  Done in 2.19s.

After:

yarn run v1.22.19
$ ~/Code/react-native-testing-library/node_modules/.bin/jest ./src/__tests__/renderHook.test.tsx
 PASS  src/__tests__/renderHook.test.tsx
  ✓ gives comitted result (5 ms)
  ✓ allows rerendering (1 ms)
  ✓ allows wrapper components

Test Suites: 1 passed, 1 total
Tests:       3 passed, 3 total
Snapshots:   0 total
Time:        0.819 s
Ran all test suites matching /.\/src\/__tests__\/renderHook.test.tsx/i.
✨  Done in 1.74s.

[pierrezimmermannbam]

Thanks for opening this pr @KrastanD! Regarding the implementation, I don't think the API of render should change because it won't work if the host names detection is not run. Also it never needs to be executed in renderHook so it does not look like we need this to be configurable since in one case we always need it and in the other one we never do. Could you update your pr to keep the public API and just make it so that renderHook doesn't run name detection?

This would be a good step forward but it would only make renderHook faster though so it doesn't fully solve the problem. I wonder if we should allow host component names to be configured by the user. It would give more control to users. On the other hand I'm not sure it should be part of the public API. I'd probably use it but it also makes the API more complex so I'm not sure it's worth it. Wdyt @mdjastrzebski @MattAgn @AugustinLF ?

[KrastanD]

Hopefully this update is more of what you had in mind @pierrezimmermannbam. I pulled out only the necessary parts of the render function into the renderHook.

I didn't want to change the api in any way so there is a bit of code duplication between updateWithAct in the render function and baseRerender in renderHook. With your blessing, I can pull out updateWithAct into its own file like renderWithAct is and reuse it in renderHook.

[mdjastrzebski]

What is the purpose of this change? It seems to do the same thing as previous code with just passed wrapper to renderWithAct

[KrastanD]

renderWithAct does what render does but without detecting hostComponentNames

[mdjastrzebski]

@KrastanD I support resolving the issue of renderHook unnecessarily calling detectHostComponents. I've encountered another use-case when it would be beneficial: in case user mocks react-native package using jest.mock() and then tests hook, the renderHook fails as Text/TestInput is not longer available (user just mocked whole react-native package), but the hook itself does not need it at all.

Current implementation feels unnecessarily complex though. I would opt for "internal" (non-documented) render flag that would control detectHostComponents method being called. By default it would be called (when user just invokes render), but when renderHook would call render it would use it to ignore that step.

Proposed naming:

• render option name: detectHostComponentNames

@pierrezimmermannbam, @KrastanD wdyt?

[KrastanD]

@mdjastrzebski My initial version of this PR which you can see here solved the problem in a similar way to what you are suggesting except instead of naming it detectHostComponentNames i named it skipHostComponentNamesConfiguration because by default it would be set to false. In @pierrezimmermannbam's comment above, he says the API of render shouldn't change which is why I rewrote it to use the internals of render, like renderWithAct, but without the hostComponentNames detection.

[pierrezimmermannbam]

@mdjastrzebski @KrastanD I personally like this implementation, I don't think it's complex, in fact I think it's an improvement. Sure there is a bit of code duplication to handle wrapper option and update with act but it doesn't add an additional prop to the render function just for renderHook's sake. This is a better separation of concern imo, if tomorrow we're to add a new feature that's only for components we'll need to add one more flag. And with this implementation it's very easy to change renderHook or render without changing the behavior of the other one, which is what we want I believe. I think this implementation will be easier to maintain so I would keep it. Also even of the flag is not documented it would be visible to users with autocomplete, it's not a big problem but still a downside

[codecov]

Codecov Report

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

Comparison is base (5bb5d2d) 97.02% compared to head (e85d685) 97.04%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1425      +/-   ##
==========================================
+ Coverage   97.02%   97.04%   +0.02%     
==========================================
  Files          68       68              
  Lines        3863     3898      +35     
  Branches      568      570       +2     
==========================================
+ Hits         3748     3783      +35     
  Misses        115      115              

Impacted Files	Coverage Δ
src/helpers/accessiblity.ts	100.00% <100.00%> (ø)
src/render.tsx	100.00% <100.00%> (ø)
src/renderHook.tsx	100.00% <100.00%> (ø)

... and 1 file 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.

[mdjastrzebski]

@KrastanD, @pierrezimmermannbam I've refactored the code so that it uses internal option detectHostComponentNames which is available internally on renderInternal function but not exposed publicly. I've also added tests that ensure proper behavior in that regard for both render and renderHook public methods.

[mdjastrzebski]

This PR has been released in v12.1.3 🚢