Signature for BoundFunction<getByRole> does not accept RegExp in 7.20.0

[sandersn]

• @testing-library/dom version: 7.20.0, broken by feat: add allowed roles to role query types #670

Relevant code or config:

Based on the tests in @testing-library/vue on Definitely Typed:

import { queries, BoundFunction } from '@testing-library/dom';
declare var y: BoundFunction<typeof queries.getByRole>
y(/some text/, {exact: true, trim: false, collapseWhitespace: true, normalizer: x => x}); // $ExpectType HTMLElement

As of 7.20.0, this gives an error on the regex /some text/. It also gives an error on the string "some text".

This is because BoundFunction is a conditional type that does not understand multiple overloads, but #670 introduced multiple overloads for getByRole in order to improve completions for ARIARole. Getting rid of the overloads fixes the problem.
has a bad interaction between the overloads of getByRole in queries.d.ts and the conditional type BoundFunction in get-queries-for-element. But the types were so complex that I didn't have time to finish debugging it.

What you did:

Running tests for Definitely Typed, including @testing-library/vue: https://dev.azure.com/definitelytyped/DefinitelyTyped/_build/results?buildId=53614&view=logs&j=b2dd9be2-fb32-52f8-c4c9-7dc0bc384587&t=f2dea62c-b696-5a51-78ad-206032e0a320&l=22288

What happened:

Error in testing-library__vue
Error: /home/vsts/work/1/s/DefinitelyTyped/types/testing-library__vue/testing-library__vue-tests.ts:69:21
ERROR: 69:21 expect TypeScript@4.0 compile error:
Argument of type 'RegExp' is not assignable to parameter of type 'ARIARole'.
Type 'RegExp' is not assignable to type '"alertdialog"'.

Problem description:

Previously, arbitrary strings, regexes and matcher functions were allowed as the first argument to BoundFunction<getByRole>. Now only literal string types that are part of ARIARole are allowed.

Suggested solution:

Just getting rid of the second overload didn't fix the problem for me; I had to create a new type alias like queries.d.ts used to have.

On the other hand, I don't know why the ComponentHarness interface in @testing-library/vue inherits from BoundFunctions in:

export interface ComponentHarness extends BoundFunctions<typeof queries> {
  container: HTMLElement;
  baseElement: HTMLBodyElement;
  debug(el?: HTMLElement): void;
  unmount(): void;
  isUnmounted(): boolean;
  html(): string;
  emitted(): { [name: string]: any[][] };
  updateProps(props: object): Promise<void>;
}

So it's possible that's not a reasonable thing to do. Maybe @cimbul can explain that — I couldn't find any documentation on BoundFunctions anywhere.

[timdeschryver]

I can reproduce this within Angular Testing Library.
It seems like the second declaration overwrites the first declaration, thus TypeScript doesn't take the type for the first declaration into account. Which is weird, because I added a test to this library to verify that you would still be able to use the first declaration.
I don't have an explanation for it... and I'm not sure if it could work with the current implementation.

However, if we want to support this feature, we could revert to the first implementation (18dcf4d).

Sorry for the inconcenience, I should've tested the current implementation within Angular Testing Library first.

[eps1lon]

You already summarized all the constraints that went into this change.

It might make sense to revisit BoundFunctions and see if we can model this closer to what's happening at runtime. Right now I don't see how all these conditionals are required. If we can't model the runtime I'd rather hardcode all the bound queries to improve readability and robustness. Optimizing for minimal code doesn't pay off long-term in my experience.

[cimbul]

@timdeschryver is there any reason a union type wouldn't work for the role argument, rather than using an overload?

export type AllByRole = (container: HTMLElement, role: Matcher | ARIARole, options?: ByRoleOptions) => HTMLElement[]

On the other hand, I don't know why the ComponentHarness interface in @testing-library/vue inherits from BoundFunctions

@sandersn, I can give some background. DOM Testing Library exports a number of element queries that take the container HTMLElement as the first argument. In the framework adapter libraries like Vue Testing Library, React Testing Library, etc., the container element is usually implied to be the component. So the testing harness exposes all the same queries with the first argument bound to the component element. They use DOM Testing Library's getQueriesForElement function to add them to the harness object. Here is the code and relevant documentation for the Vue Testing Library case.

Hopefully TypeScript 4.0's variadic tuple types can simplify BoundFunction significantly. 🤞

[cimbul]

Or, looking more closely at what you were trying to do, maybe this is better, since it doesn't allow arbitrary strings:

export type AllByRole = (container: HTMLElement, role: ARIARole | RegExp | MatcherFunction, options?: ByRoleOptions) => HTMLElement[]

[timdeschryver]

@cimbul if we do that, we don't get the autocomplete we want, see this example. The string union overrules all the other strings in the union.

Not allowing arbitrary strings would be a solution, but we want to allow it AFAIK.

fyi, I reproduced this issue in the DOM Testing Library tests here

[cimbul]

Sorry, should have looked at the history of the PR. I see you've already been down that road. Interesting find on string vs. String with the union types.

Should we revisit whether we want to allow arbitrary strings? Based on the implementation, it's never going to match if it isn't a valid ARIA role. I suppose disallowing them would be a breaking change if someone defined a utility function like:

function myUtility(role: string) {  // would need to change to "role: ARIARole"
   const myComponent = doSomething();
   return myComponent.findByRole(role);
}

I certainly don't have any objection to @eps1lon's suggestion of defining the interface for bound queries explicitly, as long as it's maintained in the same place as the unbound ones.

[timdeschryver]

I've updated my branch with the explicit queries for an element. The implementation is easier to read (as expected), and we could also remove the generic query type but that would be a breaking change.

I think the easiest way to fix this for now, would be to use the implementation with String.
We can revisit it later when there's a new major release.

[cimbul]

Making the BoundFunctions interface explicit is already a breaking change, because it no longer takes a type parameter. It also cripples the queriesToBind argument of getQueriesForElement, although I doubt that's used much in practice.

I think you're right that the ... | String approach is the only non-breaking change, but if we do make a breaking change, I'm happy to fix the VTL types. I have a couple of updates ready to go for some recent releases anyway.

[kentcdodds]

🎉 This issue has been resolved in version 7.21.1 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀