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

refactor(v13): remove detect host component names #1697

Merged
merged 2 commits into from
Nov 4, 2024

Conversation

mdjastrzebski
Copy link
Member

Summary

Remove host component name detection for v13+ release. Instead we hardcode the expected host component names & add relevant tests to make sure our isHostXxx function work correctly for RN components.

Rationale:

  1. The host component detection code gives impression of RNTL being resilient to RN mocks changes. In the past there was a change from RCTView/RCTText to View/Text. In reality that past change wasn't only about the name, but also the level at which components are mocked, as the new mocks no longer execute JS code for View, Text.
  2. RNTL is actually tightly coupled with RN mocks implementation beyond just rendered host component name, but also for their behavior. In order to properly simulate RN env, we recreate RN observed behavior inside RNTL, e.g. by checking aria-* props names, or handling pressability for Text/TextInput, as that part of the code got removed by the RN mocks. Therefore, potentially any RN mocks change will result in RNTL needing to adapt to these changes beyond the name.
  3. The current detection code supported only bases case of Text component usage. While there are people using RCTText direction for performance reasons.
  4. When switching to custom test renderer [POC] Custom test renderer #1669 , we will no longer able to render the components before knowing host component names, as the renderer itself will require configuration for which components are allowed to hold string content directly.

Test plan

All tests should pass.

@thymikee
Copy link
Member

Would be great to get rid of that indirection between incomplete RN core mocks and RNTL

@mdjastrzebski
Copy link
Member Author

RN mocks are a bigger topic that has to be coordinated closely with the RN team, as the mocks reside in the RN repo. Moreover, any changes to mock level/structure can potentially make user test bases fail. In the due time (~ RNTL v14 with the new renderer) I plan to prepare an RFC to RN core team about recommended mock changes.

Copy link
Collaborator

@pierrezimmermannbam pierrezimmermannbam left a comment

Choose a reason for hiding this comment

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

I completely agree on this, this library is anyway coupled with RN mocks so it feels like unnecessary complexity in the end

@mdjastrzebski mdjastrzebski merged commit 13bc78a into v13 Nov 4, 2024
8 checks passed
@mdjastrzebski mdjastrzebski deleted the refactor/v13/remove-detect-host-component-names branch November 4, 2024 22:20
@rtrembecky
Copy link

hi, just a quick feedback that this is missing from the v13.0.0-alpha.1 release notes, not sure if intended. I'm eager to try it to see tests performance impact

@mdjastrzebski
Copy link
Member Author

@rtrembecky good catch. Added it.

I would appreciate any feedback on v13 in your project: migration, broken tests, perf 🙏

@rtrembecky
Copy link

I've done a very quick testing on test suite of one of our RN libs - it went through fine. but unfortunately I didn't see any noticeable performance gain, which is what I'm currently interested in. (the biggest culprits for us are probably barrel files and transformation of RN flow-typed files... not related to this issue)

@mdjastrzebski
Copy link
Member Author

@rtrembecky Hmmm, that's a bit weird. I would expecte 10-20% speed improvement as in #1579. Can you share some stats? Total run time before/after, number of tests, etc?

@mdjastrzebski
Copy link
Member Author

Here are the stats for RNTL own test base

Before (7905bb5):

Test Suites: 72 passed, 72 total
Tests:       663 passed, 663 total
Snapshots:   222 passed, 222 total
Time:        9.159 s
Ran all test suites.
yarn test  52.06s user 11.12s system 555% cpu 11.366 total

After (13bc78a):

Test Suites: 72 passed, 72 total
Tests:       663 passed, 663 total
Snapshots:   221 passed, 221 total
Time:        8.074 s
Ran all test suites.
yarn test  36.39s user 7.46s system 455% cpu 9.623 total

@rtrembecky
Copy link

running a single RN test on my M1 Max takes 35s even it it's skipped, saying something about our setup - I think it's the barrel files handling (jestjs/jest#11234), so RNTL performance is most probably overshadowed. I will definitely come back, if I manage to solve this, to report the performance impact.

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.

4 participants