Replaces several DOM methods with weaker typings and closes microsoft/TypeScript#4689

[jrandolf]

The following changes have been made:

• All occurences of getElementById take an optional type parameter for manual typing.
• All occurences of getElementsByClassName take an optional type parameter for manual typing.
• parentElement now types to Element (See Node.parentElement should be Element, not HTMLElement TypeScript#4689)

[ghost]

All CLA requirements met.

[HolgerJeromin]

This will break many code parts.
Perhaps
getElementById<E extends Element = HTMLElement>(elementId: string): E | null;
will break less.

[jrandolf]

This will break many code parts.

I was thinking about this, but the spec states getElementById should return an Element. For example, SVGSVGElements. It is also simple to refactor this, given the fact we can now coerce the type via the type argument; a simple regular expression is all that is required. I wouldn't even mind developing a small CLI tool to refactor code toward this if really necessary.

IMO, if we continue to let wrong types live in this language, the future will be bleak as new interface may be impossible to implement. Considering getElementById should be used with manual typing to some extension of HTMLElement, this PR promotes a healthy dose of type-safety and readability.

[orta]

Yeah, I agree with @HolgerJeromin - I think the move to generics is a good call for usability outside of the common HTML case, but I don't think it should switch to Element.

I'll write a longer post in the original TS issue but if this switches to getElementById<E extends Element = HTMLElement>(elementId: string): E | null; then I'm cool with getting this merged.

[orta]

microsoft/TypeScript#4689 (comment)

[jrandolf]

@orta Sorry for not getting back to you after a while. I've made the changes you proposed. Your arguments are reasonable, particularly because of the possibility of a 'strict-type' module if ever necessary.

However, I did not change the parentElement type back to HTMLElement. It's not uncommon to use parentElement to retrieve an SVGElement (e.g. in scripting animated/dynamic SVG content). Moreover, coercing parentElement to SVGElement using parentElement as unknown as SVGElement is just highly unnatural.

[HolgerJeromin]

Perhaps the parent case can be more complex handled:

forreignObjectHTMLElement parent is svgSvgElement.
The rest HTMLElement parent have HTMLElement

SvgSvgElement can have svgSvgElement or HTMLElement as parent.
All the rest SvgElement have SvgElement as parents.

Or do i miss something?

[jrandolf]

Sounds reasonable! I'll make these changes.

[jrandolf]

So the changes made so far:

• All occurrences of getElementById take an optional type parameter for manual typing with default as HTMLElement.
• All occurrences of getElementsByClassName take an optional type parameter for manual typing with default using case-by-case.
  • Element.getElementsByClassName returns HTMLElement by default.
  • SVGElement.getElementsByClassName returns SVGElement by default.
  • SVGForeignObjectElement.getElementsByClassName returns HTMLElement by default.
• parentElement is case-by-case (See Node.parentElement should be Element, not HTMLElement TypeScript#4689)
  • Element.parentElement types to Element.
  • HTMLElement.parentElement types to HTMLElement.
  • SVGElement.parentElement types to SVGElement.

[HolgerJeromin]

Is this a change from upstream?

[jrandolf]

Thanks for catching this! Happened during a RegExp replace operation..

[HolgerJeromin]

Can you add similar typings to querySelector, querySelectorAll and closest?

[jrandolf]

Scope creep. There is already another discussion for that since the new template string types will change the playing field.

[orta]

Thanks!

[orta]

#970 reverts this PR - so that we can release 4.2 without this.

The revert isn't permanent, but it is too breaking to DefinitelyTyped and so it's likely to be very breaking to the rest of the TS/JS ecosystem and Anders argued that this technique is probably not what we want either way microsoft/TypeScript#42067 (comment)

It's going to need another look (and maybe we need to figure out a way to be able to determine breakability better inside PRs in this repo, because it takes a few steps to find out right now).

[felixfbecker]

It is very weird to me that the type parameter would default to HTMLElement.
document.querySelector('#id') is typed to return Element in dom.d.ts, why should document.getElementById('id') return HTMLElement by default (without any casts or type parameters from the user)? That seems incorrect and inconsistent...

I really like that the TypeScript DOM types are a trustworthy reference for how the types in the DOM actually are, often helping uncover and teaching cases that someone may not even know about (and much easier to read and more discoverable than the DOM spec). I think it would be a shame if TypeScript types moved away from modeling the correct types towards only modeling the majority-of-cases types.

It is actually very easy to accidentally write code like e.g.

document.getElementById(location.hash.slice(1))?.focus()

trusting TypeScript that it would give a type error if this was not correct and forgetting that a user-provided ID could very well match an SVG element, which would not have a .focus() method and result in a runtime Cannot call method 'focus' of undefined error. Imo, that is an error that should be caught by TypeScript, unless the user overrode it with a cast.

I understand the challenges with backwards compatibility, but as a user I'd rather update code to add casts to fix type errors. It wouldn't be the first type error in a dom.d.ts upgrade for more correct types 🙂