search

callstack / react-native-testing-library

🦉 Simple and complete React Native testing utilities that encourage good testing practices.
https://callstack.github.io/react-native-testing-library/
MIT License
3.06k stars 271 forks source link

Update ByRole queries to only accept roles #1527

Closed pierrezimmermannbam closed 1 month ago

pierrezimmermannbam commented 10 months ago

Describe the Feature

In short, I want type the role as Role instead of string | Regexp

When using byRole queries, the role is typed as string | Regexp. What's the most problematic imo is to not have any autocompletion. It takes more time but mostly it can result in tests failing because of a misspell. This could be solved by using the same trick we use for fireEvent and we could keep the same type. However I don't think any string should be allowed but only valid roles. Also I don't see valid use cases for using regexp with role queries so I don't think it should be allowed. I don't know why it's supported though so I may be missing valid use cases.
What are your thoughts on this @mdjastrzebski @MattAgn ?

Possible Implementations

Related Issues

mdjastrzebski commented 10 months ago

That seems like a good idea 🌟. We should guide the user towards proper roles.

Some points to consider:

  1. RTL seems to accept string, so we would introduce small discrepancy - I'm not too worried about it.
  2. Such change would be a breaking change, so we need to put it on hold it for v13.
  3. Perhaps for the current major version we could print a deprecation/warning if user queries with Regex or string that is not a valid role.
  4. Remember there are two types of roles, classing AccessibilityRole and aria Role. Both are supported but they have different values sometimes, we need to support both.

@pierrezimmermannbam feel free to proceed with point 3 if you have some capacity.

pierrezimmermannbam commented 10 months ago

The thing is I'm not sure we could verify if it's a valid role. For that we'd need an array with the roles but RN only exports the type afaik and we can't do runtime validation based on a type. We could however add autocompletion for valid roles before the next major but it would mean some rework. We may need to figure out first when we want to release v13 to see if it's worth it

MattAgn commented 10 months ago

I agree with the idea of stronger types. @pierrezimmermannbam you mentioned checking roles a runtime, but isn't type checking enough of a safety net?

pierrezimmermannbam commented 10 months ago

@MattAgn it is, I was referring to what @mdjastrzebski was suggesting, that we could issue warnings before publishing the next major

mdjastrzebski commented 10 months ago

@pierrezimmermannbam , @MattAgn: Types vs runtime dichotomy is real, I've browser RN source but they do not seem to export runtime values for allow functions.

My idea about detecting it at runtime was intended for v12 improvement until we are ready to ship v13 with changes types which would be a breaking change. If that is not easily achievable, let's wait with typing change till v13.

MattAgn commented 10 months ago

I agree with you, and I don't think many users use roles that are neither AccessibilityRole and aria Role. And if they do, I guess it should not be too hard to fix their tests to fit the new typing

mdjastrzebski commented 6 months ago

@pierrezimmermannbam @MattAgn reviving this old improvement idea. What about doing TS hack on suggesting the proper roles for v12, and then potentially restricting it for v13?

pierrezimmermannbam commented 6 months ago

Yes that definitely already be a huge improvement, it's really the autocomplete that is missing rn. This is also fairly easy to do, I could open a pr this week

mdjastrzebski commented 5 months ago

Merged #1596 with autocomplete.