Accept type argument to getElementById

[Sainan]

This provides an alternative to the common pattern of document.getElementById("...") as HTMLWhateverElement which casts away the possibility of null, consequently hiding a potential type error.

[github-actions[bot]]

Thanks for the PR!

This section of the codebase is owned by @saschanaz - if they write a comment saying "LGTM" then it will be merged.

[Sainan]

Might be worth pointing out npm run baseline-accept in the "contribution guidelines" 🍞

[saschanaz]

I think the guideline has been that, if you are not actually using the type parameter in the signature then it should not exist, and rather you should cast from the return type. @jakebailey, is that still the case?

[jakebailey]

Yes, this effectively a type assertion in disguise. It's not good to have this kind of signature. An explicit assertion is much preferred.

[jakebailey]

I understand the risks but I don't think this is a good idea.

(If we were to have something like this, it'd at least have to be T extends ... = ...

[Sainan]

My case for it: querySelector and querySelectorAll have it. It is very common for the T to be a type assertion (e.g. JSON.parse)

Also updated to require that T extends HTMLElement

[jakebailey]

My case for it: querySelector and querySelectorAll have it.

I have no idea why these have it, I doubt we would choose to do so if the same decision were made today. It's patently unsafe and hides the type assertion from tools that intend to find unsafety.

It is very common for the T to be a type assertion (e.g. JSON.parse)

JSON.parse doesn't do this? Unless you're talking about third party patches?

[Sainan]

I have no idea why these have it, I doubt we would choose to do so if the same decision were made today. It's patently unsafe and hides the type assertion from tools that intend to find unsafety.

tbh I have no idea about the tooling landscape here (my version of Vite is called PHP) but to me a ! seems more explicit than the as ... just casting away the | null

JSON.parse doesn't do this? Unless you're talking about third party patches?

My mistake, the typing I got is from json-with-bigint

[saschanaz]

querySelector has it because if you do querySelector("input") then you know it's going to be HTMLInputElement. Not much for getElementById.

[Sainan]

I pinky promise I know what my .html/.php file looks like 😄

[jakebailey]

querySelector has overloads that have nice return types, but then they end with:

querySelector<E extends Element = Element>(selectors: string): E | null;
querySelectorAll<E extends Element = Element>(selectors: string): NodeListOf<E>;

I'm not sure if that's because it's required to make the overload checks happy, but I don't think so 😦

[jakebailey]

I pinky promise I know what my .html/.php file looks like 😄

That same argument could apply to all of TS 😄

"I know better than the compiler" is supposed to be casts and !, not type vars... But I understand the tension and the nullness problem here.

[Sainan]

Example code for your consideration:

The relevant part:

document.getElementById("filter-" + opt) as HTMLInputElement | null

Could be written currently as:

document.querySelector<HTMLInputElement>("#filter-" + opt)

But with this PR;

document.getElementById<HTMLInputElement>("filter-" + opt)

[HolgerJeromin]

For me the added null is a very good thing, In our codebase are some as HTMLElement after getElementById where the junior devs forgot (and therefor dropped) the null.

With this PR we could migrate to the generics and will get the null for free.