Dom-testing-library: Add generics to get/query methods

[nstepien]

Describe the feature you'd like:

I know the element type I'm querying, so it'd be beneficial to specify it upfront:

screen.queryByText<HTMLButtonElement>('click me'); // -> HTMLButtonElement | null

Type assertions are very error prone, so should be avoided:

screen.queryByText('click me') as HTMLButtonElement; // can still be null!

Suggested implementation:

queryByText<T extends HTMLElement = HTMLElement>(...) => T | null;

Describe alternatives you've considered:

You can do already do something similar with for example querySelector:

document.body.querySelector<HTMLButtonElement>('.my-button');

Teachability, Documentation, Adoption, Migration Strategy:

[kentcdodds]

Hi @nstepien,

Interestingly, when Testing Library was released, it only supported the query* variant. But someone opened an issue about this with TypeScript and we introduced get* which can only return the element (otherwise it throws). So while I'm not certain of your specific suggestion (I don't use typescript), I can suggest that you switch to get* and avoid the issue you're talking about.

[nstepien]

@kentcdodds I might have not made myself clear, sorry.
What I mean is that queryByText for example returns HTMLElement | null by default, which makes accessing button.disabled a type error, so I have to assert the type of the returned value to HTMLButtonElement | null.
So my issue is that all the get/query methods have a return value of type HTMLElement (+ | null for queries) and require me to assert the type, which is error prone.

[kentcdodds]

Ah, gotcha. There's currently an effort to migrate this project to TypeScript and maybe when that's finished we can address this. It definitely makes sense.

[smeijer]

@kentcdodds, @nstepien is referring purely to the typescript annotations. If I check the ones that are now at definitely typed, they indeed do not support generics.

I'm with @nstepien here, generics would be definitely helpful. It's a way to hint typescript about the return type of the function. Instead of casting it to a given one afterward.

// hinting / declaring
screen.queryByText<HTMLButtonElement>('click me'); // -> HTMLButtonElement | null

// casting
screen.queryByText('click me') as HTMLButtonElement; 

The first one can return type errors if the resulting element isn't a button. The second will forcefully declare it as being one. Possibly resulting in runtime errors later on.

This same story applies to the other methods, find, get and the all variants.

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/9fdeede40caaef5e431462f140bec1d3383c0ab9/types/testing-library__dom/queries.d.ts#L68-L86

[eps1lon]

~Definitely going to strongly advocate for type casting here. More explanation in https://twitter.com/SeaRyanC/status/1090998621472940032~

~TL;DR: Using generics in this case is just like type casting without the explicitness of type casting which makes it strictly worse.~

Update: See https://github.com/testing-library/dom-testing-library/issues/615#issuecomment-655411294 for current opinion.

[eps1lon]

screen.queryByText('click me') as HTMLButtonElement; // can still be null!

That convinced me to go with generics instead. I wish we had a better mechanism because this still masquerades type casting but it's somewhat safer type casting.

[leomeloxp]

I think the most appropriate way to supporting generics in here would be to have it extend HTMLElement or fallback to it, eg:

// ...
export type QueryByBoundAttribute<T extends HTMLElement> = (
  container: HTMLElement,
  id: Matcher,
  options?: MatcherOptions,
) => T | null
// ...
export type GetByBoundAttribute<T extends HTMLElement> = (
  container: HTMLElement,
  id: Matcher,
  options?: MatcherOptions,
) => T
// ...

[Daniel-Griffiths]

I agree with @leomeloxp ‘s solution.

Using type casting allows the user to potentially set incorrect types and hit undefined errors at runtime.

Heres an example:

Using casting

// Type is incorrectly HTMLImageElement
const element = screen.queryByTestId(‘some-id-that-doesnt-exist’) as HTMLImageElement; 

// We get a runtime error, .src does not exist on element
doSomething(element.src)

Using generics

// Type is correctly HTMLImageElement | null
const element = screen.queryByTestId<HTMLImageElement>(‘some-id-that-doesnt-exist’); 

// Error in your code editor/build step telling you to check if "element" is defined before using it
doSomething(element.src) 

We could argue that developers can cast thier code by using as HTMLImageElement | null but it's easy to forget, and not everyone will know they need to do it. Using generics will enforce the | null on the return type and help prevent runtime errors.

[eps1lon]

Using type casting allows the user to potentially set incorrect the types and hit undefined errors at runtime.

So would using generics. Though arguing is moot since all the points you brought up were already included in the initial description.

[nstepien]

@Daniel-Griffiths did you read the opening post?

[nickmccurdy]

From what I've heard about writing type declarations, using generics only to set the return type without checking or inferring against any other types (such as parameters in an endofunctor) is an antipattern. Assertions/casts would be preferred in this case.

[nstepien]

I don't know what it is you read about it, but you can't validate that your type assertion extends HTMLElement for example, but you can with generics. It's too easy to make mistakes with type assertions, like has been discussed in this thread.

[timdeschryver]

With TS 4.1, we could provide type-safety for the ByRole queries.
We can infer the element type by using the aria role, something like this:

export type ExtractElement<
  RoleMatcher extends ByRoleMatcher
> = RoleMatcher extends ARIARole
  ? RoleMatcher extends `button`
    ? HTMLButtonElement
    : RoleMatcher extends `textbox`
    ? HTMLInputElement | HTMLTextAreaElement
    : RoleMatcher extends `checkbox`
    ? HTMLInputElement
    : HTMLElement
  : HTMLElement

This seems to work in my POC, but I think my current implementation can be refactored a bit.
If you think this is a good enhancement, I can create a Pull Request and we can discuss it there.

[eps1lon]

@timdeschryver A role="button" can be any Element not just HTMLButtonElement. Same applies to most roles.

[timdeschryver]

Right 🤦‍♂️, thanks for the reminder.