Skip to content

[typescript] lib.dom.d.ts has confusing types #23595

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
DovydasNavickas opened this issue Apr 20, 2018 · 6 comments
Closed

[typescript] lib.dom.d.ts has confusing types #23595

DovydasNavickas opened this issue Apr 20, 2018 · 6 comments
Labels
Declined The issue was declined as something which matches the TypeScript vision Suggestion An idea for TypeScript

Comments

@DovydasNavickas
Copy link

TypeScript Version: 2.8.1
VS Code Version:

Version 1.22.2
Commit 3aeede733d9a3098f7b4bdc1f66b63b0f48c1ef9
Date 2018-04-12T16:38:45.278Z
Shell 1.7.12
Renderer 58.0.3029.110
Node 7.9.0
Architecture x64

Search Terms:
useCapture
addListener
lib.dom.d.ts

Code

const htmlElement: HTMLDivElement;
htmlElement.addEventListener("scroll", event => console.log(event), true);

Before entering the last parameter (just after writing the last comma in the editor):

htmlElement.addEventListener("scroll", event => console.log(event), <----- When the cursor is here

I get code completion that looks like this:

image
And the second overload:
image
It is very confusing to see options?: boolean part not as a separate overload, but as a smushed definition of:

options?: boolean | AddEventListenerOptions | undefined

Expected behavior:
To have separate overloads for boolean version and AddEventListenerOptions one, as seen in CodeSandbox (Playground link).

Actual behavior:
Described above.

Playground Link:
https://codesandbox.io/s/r0l6j9olpn

Related Issues:
Didn't find.

The interesting thing is that while trying to reproduce the issue in CodeSandbox, I got exactly what I wanted to see: useCapture: boolean | undefined
image

So, my question is why does VS Code have a version of types that is confusing and can also be found here:
https://github.com/Microsoft/TypeScript/blob/master/lib/lib.dom.d.ts#L5450

And why CodeSandbox has a proper one? And how can we solve this?
Is this VS Code's bug or the lib.dom.d.ts bundled with TypeScript is simply out-of-date?

@DovydasNavickas
Copy link
Author

Hmmm.... I see that there already is a related issue: [typescript] dom definitions not up to date #19040

The sad thing is that it is not solved since Oct 9, 2017, but CodeSandbox has done something about it already, so there must be a version of lib.dom.d.ts that is newer.
But TypeScript bundles a version that is out of date and we all know that confusing types might be worse than no types 🙂 How can we solve this once and for all?
Maybe including lib.dom.d.ts into DefinitelyTyped community's responsibility would break the ice on this one?
I've contributed to quite a few package types there directly and with colleagues, code and discussions, reviewing PRs and approving or declining them (react being the biggest and most used one) and no matter the size of the types, they evolve to better state over time and are usually up to date. With lib.dom.d.ts, I have no idea how to properly contribute and how can there be better and newer versions out there (CodeSandbox case), when TypeScript's repo one is out of date for years?

@DovydasNavickas DovydasNavickas changed the title lib.dom.d.ts [typescript] lib.dom.d.ts has confusing types Apr 20, 2018
@mhegazy
Copy link
Contributor

mhegazy commented Apr 20, 2018

The library is generated from https://github.com/Microsoft/TSJS-lib-generator. You can find more information about contributing lib.d.ts fixes at https://github.com/Microsoft/TypeScript/blob/master/CONTRIBUTING.md#contributing-libdts-fixes.

@mhegazy
Copy link
Contributor

mhegazy commented Apr 20, 2018

So the suggession here is to have two overloads?

interface HTMLDivElement extends HTMLElement {
    addEventListener<K extends keyof HTMLElementEventMap>(type: K, listener: (this: HTMLDivElement, ev: HTMLElementEventMap[K]) => any, options?: boolean): void;
    addEventListener<K extends keyof HTMLElementEventMap>(type: K, listener: (this: HTMLDivElement, ev: HTMLElementEventMap[K]) => any, options?: AddEventListenerOptions): void;

    addEventListener(type: string, listener: EventListenerOrEventListenerObject, options?: boolean ): void;
    addEventListener(type: string, listener: EventListenerOrEventListenerObject, options?: AddEventListenerOptions): void;

    removeEventListener<K extends keyof HTMLElementEventMap>(type: K, listener: (this: HTMLDivElement, ev: HTMLElementEventMap[K]) => any, options?: boolean): void;
    removeEventListener<K extends keyof HTMLElementEventMap>(type: K, listener: (this: HTMLDivElement, ev: HTMLElementEventMap[K]) => any, options?: EventListenerOptions): void;

    removeEventListener(type: string, listener: EventListenerOrEventListenerObject, options?: boolean): void;
    removeEventListener(type: string, listener: EventListenerOrEventListenerObject, options?: EventListenerOptions): void;
}

@mhegazy mhegazy added the Needs More Info The issue still hasn't been fully clarified label Apr 20, 2018
@DovydasNavickas
Copy link
Author

Yes, except boolean parameter should be called useCapture for clarity.

interface HTMLDivElement extends HTMLElement {
    addEventListener<K extends keyof HTMLElementEventMap>(type: K, listener: (this: HTMLDivElement, ev: HTMLElementEventMap[K]) => any, useCapture?: boolean): void;
    addEventListener<K extends keyof HTMLElementEventMap>(type: K, listener: (this: HTMLDivElement, ev: HTMLElementEventMap[K]) => any, options?: AddEventListenerOptions): void;

    addEventListener(type: string, listener: EventListenerOrEventListenerObject, useCapture?: boolean ): void;
    addEventListener(type: string, listener: EventListenerOrEventListenerObject, options?: AddEventListenerOptions): void;

    removeEventListener<K extends keyof HTMLElementEventMap>(type: K, listener: (this: HTMLDivElement, ev: HTMLElementEventMap[K]) => any, useCapture?: boolean): void;
    removeEventListener<K extends keyof HTMLElementEventMap>(type: K, listener: (this: HTMLDivElement, ev: HTMLElementEventMap[K]) => any, options?: EventListenerOptions): void;

    removeEventListener(type: string, listener: EventListenerOrEventListenerObject, useCapture?: boolean): void;
    removeEventListener(type: string, listener: EventListenerOrEventListenerObject, options?: EventListenerOptions): void;
}

And there are more occurrences of this parameter type, not only HTMLDivElement.

To be more precise, 393 occurrences of this particular type definition.
image

If you know how to resolve this with https://github.com/Microsoft/TSJS-lib-generator in a quickly, it would be amazing to have this fix throughout all lib.dom.d.ts 🙂

@mhegazy mhegazy added Suggestion An idea for TypeScript Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript and removed Needs More Info The issue still hasn't been fully clarified labels Apr 21, 2018
@DeividasBakanas
Copy link

I can confirm that useCapture would have helped a lot... Spent quite some time thinking:

What is that options: boolean thing? And how do I set useCapture?

Do you plan updating the lib.dom.d.ts?

@RyanCavanaugh RyanCavanaugh added Declined The issue was declined as something which matches the TypeScript vision and removed Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Domain: lib.d.ts The issue relates to the different libraries shipped with TypeScript labels Jun 25, 2021
@RyanCavanaugh
Copy link
Member

Changing a union to overloads is usually much worse from a type system perspective, since it means you can't forward same-signature parameters around. The upside in documentation clarity doesn't seem worth making the types worse here given the low volume of feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Declined The issue was declined as something which matches the TypeScript vision Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

4 participants