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

Update ByRole queries to only accept roles #1527

Open
pierrezimmermannbam opened this issue Nov 14, 2023 · 9 comments
Open

Update ByRole queries to only accept roles #1527

pierrezimmermannbam opened this issue Nov 14, 2023 · 9 comments

Comments

@pierrezimmermannbam
Copy link
Collaborator

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
Copy link
Member

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
Copy link
Collaborator Author

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
Copy link
Collaborator

MattAgn commented Nov 14, 2023

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
Copy link
Collaborator Author

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

@mdjastrzebski
Copy link
Member

@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
Copy link
Collaborator

MattAgn commented Nov 20, 2023

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
Copy link
Member

@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
Copy link
Collaborator Author

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
Copy link
Member

Merged #1596 with autocomplete.

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

No branches or pull requests

3 participants