feat: config option to allow breaking changes

[mdjastrzebski]

Summary

New config option allowBreakingChanges that when set to true would allow us to introduce breaking changes without need for major release. It has to be opt-in, so that if not set the library works in non-breaking way.

The goal here is to be able to move forward with code changes that introduce breaking changes like #1234 #1180 without blocking other non-breaking developments. When the major release happens we would promote changes enabled by allowBreakingChanges optiopn and make the the default.

I've also have alternative names for the option:

• useCuttingEdge: true
• useLatestApis: true
• useVersionNext: true
• moveFastAndBreakThings: true

Pls treat this PR as RFC/discussion I've submitted it as PR because it was low-effort to do it in this way. @pierrezimmermannbam @AugustinLF @MattAgn @thymikee wdyt?

Test plan

No new changes, as the option is not yet used anywhere.

[thymikee]

I don't think this is a good idea. It will require some extra documentation and care from our side. I'd stick with separate flags

[mdjastrzebski]

@thymikee as a counter argument I would say that maintaining a number of separate options seem to require even more extra documentation and care from our side.

Important part of this idea is that after major release we are able to remove the legacy implementations and just stick with the new ones. I do not plan for these to stick around after the major release.

[thymikee]

So it's for early adopters only. Maybe it's worth to spin a new branch and start the RC phase instead?

[mdjastrzebski]

Yeah the goal here is that we have some breaking changes intended for next major release:

• feat(breaking): Return host component for all queries #1234
• *ByRole: return only elements with implicit or explicit accessible={true} prop #1180

and we want to be able to work on these in an effective manner.

Merging any of these PRs without any flags would be a breaking change, so will put us in this vNext preparation state where we will not be able to do non-breaking features (without branching off to 11.x branch). Other the other hand it would put pressure on us to end this phase relatively soon not to block potential incoming non-breaking features/bug fixes/etc.

Branching seems to be one solution, so we could either do branch for existing 11.x version and do breaking stuff on main OR we could do other way around continue non-breaking stuff on main and create vNext version. The downside of both of these approaches is that we would have to do merges/rebases/cherry picks/etc.

It seems like breaking changes intended for v12 are not too deep so hiding them behind this "breaking changes" feature flag would allow us that keep a single code base, with some ifs and duplications. When the time for release comes, we would just remove the no longer used legacy code paths which should be relatively easy process.

From users perspective this would allow early adopters to use these features quicklier.

An interesting variation of that approach would be to keep the allowBreakingChanges flag internal so users would not be aware of that but we would keep both breaking changes + legacy code with unit tests for both version. Wdyt?

[MattAgn]

I like the idea of having just one main branch and only one flag to handle the breaking feature, seems more simple to handle.

What I'm wondering is wether people are sufficiently investing in testing in general to use the flag allowBreakingChanges. Many of the new features are often used several months (up to a year I'd say) after they are released because teams don't put that much emphasis on tests. Do you have a similar feeling?
So imo we can keep the allowBreakingChanges internal because I feel very few people would actually use that option. That will save us some code and some documentation to write

[pierrezimmermannbam]

I agree that we have a lot of complexity right now due to dealing with breaking changes, so having an internal feature flag only for release management seems like a good idea to me! This offers less possibilities though and may not be the best solution when we want users to have the possibility to disable some feature. So regarding the next major version I think we need to figure out wether the breaking changes should be mandatory or not and if users should be able to have one and not the other.

Regarding #1234, I believe we should keep the option to disable it at query level but I like the idea of using allowBreakingChanges as global flag. Not sure what's planned for #1180, but it could follow the same pattern, be an option on byRole queries and depending on allowBreakingChanges flag. Either that or not even configurable at query level but I'm not sure how big of a breaking change it is.

[mdjastrzebski]

Re mandatory/optional changes in the next major release:

#1234
I think we should make #1234 mandatory in the next major release. Main reason would be to return only host elements from all queries. These are the elements representing native UI controls, and it make sense to check assertions on these elements, and not their composite counterparts. Having queries returned composite elements caused various edge-case issues with other queries (*ByRole + within) and fireEvents (TextInput checks), which would be avoided.

Secondarily, composite and host Text & TextInput seem to pass props using object spreading with little transformation. So in general case there should not be problems with migrations unless user are checking some exotic props and/or navigating through the component tree (which we don't support anyway).

From what I understand the current code which uses exported composite Text & TextInput is result of RN changing host names of these components in the past from RCTText => Text, which did/could broke RNTL queries in the past.

#1180
I do not have strong opinion here, I think it would be good to do some more checks, see how current tests react to the code changes, etc. That change is from "better env simulation" theme and is based on how screen readers interacts with elements. If we decide it make sense to make it available after release as compat option, we could name it something like strict: true (as in "strict mode"). On the other hand if this change highlights the cases where users are using accessibilityRole incorrectly, so that it wouldn't be captured by screen reader then we might want to make it mandatory, so that they just add accessible={true} where necessary.

@pierrezimmermannbam @MattAgn wdyt?

[MattAgn]

I agree with you on both options.

For #1180, I guess we'll know more when I do my research on which components are considered accessible by default by screen readers.

[thymikee]

Ok, so I can see how this could make your lives easier. In such case, I'm not opposed

[pierrezimmermannbam]

@mdjastrzebski I also agree, should we merge this pr first before I make changes on #1234 ? Regarding the implementation I guess what we'd need would be the equivalent of the existing config but only for internal purposes, as we'd need the ability to changes the flag's value in tests as well as reset the value to the default

[codecov]

Codecov Report

Base: 94.91% // Head: 94.94% // Increases project coverage by +0.03% 🎉

Coverage data is based on head (541eb2b) compared to base (6c606ba).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1241      +/-   ##
==========================================
+ Coverage   94.91%   94.94%   +0.03%     
==========================================
  Files          45       45              
  Lines        3069     3088      +19     
  Branches      456      457       +1     
==========================================
+ Hits         2913     2932      +19     
  Misses        156      156              

Impacted Files	Coverage Δ
src/config.ts	100.00% <100.00%> (ø)

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

[mdjastrzebski]

@pierrezimmermannbam @MattAgn @thymikee: I've made allowBreakingChanges an internal option so it won't be visible to the users. Pls review.

Re merge order for #1234 and #1180: I think it would be good idea to merge this PR first so that your PRs can already use it.

[mdjastrzebski]

@pierrezimmermannbam let me know if you have any comments to the existing implementation or should I merge it?

[mdjastrzebski]

No comments from @pierrezimmermannbam, so I'm merging this PR to move forward.

[pierrezimmermannbam]

@mdjastrzebski sorry I didn't take the time to review this week but it's perfect !