Skip to content

fix querySelector typings #357

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
wants to merge 1 commit into from

Conversation

k1r0s
Copy link
Contributor

@k1r0s k1r0s commented Jan 25, 2018

No description provided.

@HolgerJeromin
Copy link
Contributor

Why is that PR needed?
document.querySelector("svg")
returns the first SVGSVGElement of the document. This is not an instance of HTMLElement.

@k1r0s
Copy link
Contributor Author

k1r0s commented Jan 25, 2018

This method is overloaded, other elements can be retrieved with that method, not only SVG instances. My PR fix wrong typings when retriving other elements such as <div>, <span> .. whatever. Basically typings retrieve the wrong Type which missmatch with the retrieved runtime instance.

@mhegazy
Copy link
Contributor

mhegazy commented Jan 29, 2018

This method is overloaded, other elements can be retrieved with that method, not only SVG instances.

but you removed the generic one that was a catch-all for both HTMLElement and SVGElement.

I am not sure i understand the issue, can you elaborate on what this change achieves?

Also for future references, please file a bug under https://github.com/Microsoft/TypeScript/issues first.

@k1r0s
Copy link
Contributor Author

k1r0s commented Jan 29, 2018

Problem is that Element doesn't contain some of properties of HTMLElement, say style for instance. I was wondering how to achieve this. I wasn't aware of SVGElement.

Any comments? maybe problem is on Element interface :\

@saschanaz
Copy link
Contributor

You can do querySelectorAll<HTMLElement>(".if .you .need").

@k1r0s
Copy link
Contributor Author

k1r0s commented Jan 30, 2018

IMO using generics isn't a good solution.

@k1r0s
Copy link
Contributor Author

k1r0s commented Jan 30, 2018

By the way HTMLElement as a generic doesn't work either bcoz you have to specificly say what type you expect

@k1r0s
Copy link
Contributor Author

k1r0s commented Jan 30, 2018

I just checked that both HTMLElement and SVGSVGElement have an style property which should be and abstract one on Element, which now doesn't exist.

The real problem is on Element class

@k1r0s
Copy link
Contributor Author

k1r0s commented Jan 30, 2018

Maybe I should close this PR and create another

$ inputfiles/addedTypes.json

    {
        "kind": "property",
        "interface": "Element",
        "name": "style",
        "type": "CSSStyleDeclaration"
    },

@HolgerJeromin
Copy link
Contributor

HolgerJeromin commented Jan 30, 2018

@k1r0s Element is no TypeScript invention!
It is defined in DOM.
You see Element as equivalent to HTMLElement | SVGElement which is the reality for current Browsers, but not according to the standard.

@k1r0s k1r0s closed this Jan 30, 2018
@k1r0s k1r0s deleted the fix-querySelector-typings branch January 30, 2018 15:08
@k1r0s
Copy link
Contributor Author

k1r0s commented Jan 30, 2018

@HolgerJeromin I get your point.

Because generics aren't working with that signature

The only workarround is to use a Type alias. document.querySelector(".hello") as HTMLElement

And isn't enough for me

@HolgerJeromin
Copy link
Contributor

@k1r0s
document.querySelector<HTMLElement>(".hello")
is better than the type assertion.

@k1r0s
Copy link
Contributor Author

k1r0s commented Jan 30, 2018

@HolgerJeromin try it by yourself. Isn't working because you have this signature:

querySelector<K extends keyof ElementTagNameMap>(selectors: K): ElementTagNameMap[K] | null

@HolgerJeromin
Copy link
Contributor

@k1r0s I did with ts2.6.3 with strictNullCheck active

let hello = document.querySelector<HTMLElement>(".hello");
hello is HTMLElement|null and transpiled JS is
var hello = document.querySelector(".hello");

@k1r0s
Copy link
Contributor Author

k1r0s commented Jan 31, 2018

yep @HolgerJeromin didn't try latest version. Thanks

@HolgerJeromin
Copy link
Contributor

@k1r0s This is new with 2.6

@k1r0s
Copy link
Contributor Author

k1r0s commented Jan 31, 2018

very nice. Indeed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants