refactor: type all queries to return host components

[pierrezimmermannbam]

Summary

• Updates the return type of all queries to HostReactTestInstance
• Moves logic of filtering non host elements in findAllInternal rather than on each query level

Now that all queries return host elements, this can be made explicit by a more precise typing. It also ensures that queries will consistently return host elements from now on. I also moved the logic of filtering non host elements in findAllInternal rather than on each query level to remove some code duplication and most importantly remove checks that didn't use isHostElement and therefore were not explicit.

Test plan

All existing tests and type checks pass

[mdjastrzebski]

@pierrezimmermannbam I've got back from my summer break. Great to see your` PRs!

Regarding this particular PR, the idea of using host check in findAll definitely makes sense. There are still two queries that can return composite elements (UNSAFE_*) but they do not use the same findAll implementation.

I am a bit concerned with the HostTestInstance type, as I recall that in the past I've tried to do something similar but had encountered some TS errors, as React typed type prop with HTML string literal union type ('a' | 'div' | ...), etc with somehow clashed with some other types. Unfortunately I do not remember the details. Perhaps TS and/or React types have improved. I will try to do some experiments on your PR to see if I can replicate the issues.

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.02% ⚠️

Comparison is base (62a9b57) 97.57% compared to head (99ee190) 97.56%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1426      +/-   ##
==========================================
- Coverage   97.57%   97.56%   -0.02%     
==========================================
  Files          74       73       -1     
  Lines        4338     4312      -26     
  Branches      636      627       -9     
==========================================
- Hits         4233     4207      -26     
  Misses        105      105              

Files Changed	Coverage Δ
src/helpers/matchers/matchLabelText.ts	100.00% <ø> (ø)
src/fireEvent.ts	100.00% <100.00%> (ø)
src/helpers/component-tree.ts	100.00% <100.00%> (ø)
src/helpers/findAll.ts	100.00% <100.00%> (ø)
src/helpers/host-component-names.tsx	100.00% <100.00%> (ø)
src/queries/a11yState.ts	100.00% <100.00%> (ø)
src/queries/a11yValue.ts	100.00% <100.00%> (ø)
src/queries/displayValue.ts	100.00% <100.00%> (ø)
src/queries/hintText.ts	100.00% <100.00%> (ø)
src/queries/labelText.ts	100.00% <100.00%> (ø)
... and 5 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mdjastrzebski]

@pierrezimmermannbam I've done some experiments on this PR and here are some interesting findings:

1. Your PR seems to work fine in the current scope.
2. ReactTestInstance['type'] is indeed a string literal union of HTML tag names + class and function components:

So React incorrectly assumes that host components would be only HTML tags. Ignoring the React Native situation.

3. That means that HostReactTestInstance type is actually intersection of string and above string literal union of HTML tags:

With TS errors like:

Argument of type '"Image"' is not assignable to parameter of type 'ElementType & string'.

Now part for the questions:

1. I wonder how serious issue would that type being slightly incorrect be actually be in the real world.
2. We should consider is changing the external types to the new type could be a breaking change for any of our users.

@pierrezimmermannbam looking forward for some thoughts on that.

[pierrezimmermannbam]

I don't think it's a big problem that type is incorrectly typed. It was already typed incorrectly before this PR (even more so) and we haven't gotten complaints afaik. It could be a problem for users if they try to directly access the type property and check if e.g. type is 'Text' in which case they'd probably get an error saying that that's impossible. I don't really see a use case for it though and I don't think a lot of users would do that. We could change the type of the public API for the users but I don't think it's worth it, especially since we don't really know what can the host component names can be except for those we use in queries.

This PR would technically be a breaking change since it narrows the return type of queries. It should have very little impact though but still I think it's probably better to release it with the next major version. Also this PR mostly brings benefits for the code maintenance but holds little to no value for users so there's no real inconvenience in delaying its release

[mdjastrzebski]

I think that returning covariant (more specialized) type from query functions should not be considered a breaking change, as any variable assignments that user has would still be valid, as HostInstance is subtype of ReactTestInstance.

As for TS errors, I've did some checks and using checks like elements === 'Text' does not rise TS warnings nor errors. It is only problematic in cases when we would want to pass View, Text, etc to function accepting the TS type derived from ReactTestInstance but that would be also problematic in the first case.

The one decision I am still contemplating is whether we should make the host type information public or keep it internal only. I think that we should rather keep this internal for now, as it would bring some benefits to us in terms of code simplification/expressiveness, while it could be considered confusing for the users and would require proper documentation, etc.

[pierrezimmermannbam]

@mdjastrzebski if I understand correctly you're suggesting that we don't change the public API at all? Or that we only change the naming of the exposed type?

[mdjastrzebski]

@pierrezimmermannbam yes, I am proposing to use the HostReactTestInstance inside the library, as it simplifies the code, but not to expose this type outside, and declare returning ReactTestInstance as we did so far.
The reasoning behind is is that it will complicate our documentation and learning curve for new users, while most of the them should just be using proper Jest matchers instead of going into implementation details like this.

[pierrezimmermannbam]

@mdjastrzebski I agree with your motives, keeping the same public API would indeed make things simpler for users. What bothers me with your changes is that we no longer are certain that queries return host components. I pushed an alternative version that adds an internal type for queries so that we're sure every query return host components while keeping the same public API. It does add a bit of complexity but it seems like a fair price to pay, wdyt?

[mdjastrzebski]

@pierrezimmermannbam It seems we are not using queries internally in a way that having return type of HostReactTestInstance would benefit the code. Do you know any such cases? If not then perhaps we could just leave the return type on queries as regular ReactTestInstance?

[pierrezimmermannbam]

@mdjastrzebski the return type is indeed not used in the code so specifying it doesn't simplify the code but the true motive here is specifying the type. I think that we want all queries to return host components and that typing enforces that. It's like typing the public API, only it's hidden

[mdjastrzebski]

Looks good. I've reverted the last commit on InternalQueryAll types, and renamed HostReactTestInstance to a tad simpler HostTestInstance`. I've also done some minor cleanup in existing code.

[mdjastrzebski]

This PR has been released in v12.2.1 🚡