feat(breaking): Return host component for all queries #1234
Conversation
pierrezimmermannbam
requested review from
mdjastrzebski,
MattAgn and
AugustinLF
Codecov ReportBase: 94.94% // Head: 95.05% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1234 +/- ##
==========================================
+ Coverage 94.94% 95.05% +0.11%
==========================================
Files 45 46 +1
Lines 3088 3178 +90
Branches 457 474 +17
==========================================
+ Hits 2932 3021 +89
- Misses 156 157 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
@pierrezimmermannbam awesome job, I will try to review it at the beginning of the week! |
There was a problem hiding this comment.
I was thinking about how can we reconcile semver breaking changes vs moving forward in anticipation of upcoming major release. My idea would be to create a global configure option called allowBreakingChanges that when set explicitly to true would run code that might introduce breaking changes. Then on major release we would remove all !allowBreakingChanges code paths.
This is similar to what you propose in this PR however in a bit more generalised way, that would accommodate other breaking changes we want to make soon like #1180. @pierrezimmermannbam @MattAgn @AugustinLF wdyt?
pierrezimmermannbam
force-pushed
the
poc/hostComponentQueries
branch
from
91409c2 to
f08715a
Compare
|
@mdjastrzebski @MattAgn I reworked the pr to use the useBreakingChange flag and treated all your comments |
There was a problem hiding this comment.
Awesome refactor @pierrezimmermannbam 🚀
I really like the simplified code structure. I think that the key ingredients of this PR are already done. There are some relatively minor issues that need to be addressed before merging though. Looking forward to getting this key refactor merged which will bring us close to vNext. 🎸
thymikee
changed the title
|
@mdjastrzebski I made the final modifications, it should be ready to merge! |
Summary
Closes #1132 #1184
Changes behavior of byText, byDisplayValue and byPlaceholderText queries so that they return host components instead of composite components.
This new behavior is activated by default but can be disabled either globally through the config useLegacyQueries option or per query through the legacy option.
When returning host components, the queries will read the component names from the config. The defaults are 'Text' and 'TextInput'. If a newer RN version changes these names we could support the newer version by changing the default based on the RN version used by the user but for now all RN versions supported by RNTL have the same name for Text and TextInput components.
Users are able to change the host component names by using the configure API. When a getBy query fails, we detect the true host component names used by RN and if they do not match the ones in the config suggest the user to either update RNTL or modify its config with the names we found.
This is a breaking change so it should be included in some next major release. I haven't done it yet but a migration guide should be written before releasing the next major version. Also the documentation on testing environment will need to be updated as it states that those queries currently return composite components.
Test plan
I added test cases for each impacted queries with all combinations of options and tested as well that they correctly use the host component names from the config and that they throw the expected error message when they fail and the names used do not match the ones from React Native.