Should the $ function return HtmlElement?

[yebrahim]

Now it's typed to return a Polymer.Element, which almost always is not going to be the case. Why doesn't it just return an HTMLElement and let the user cast it if needed?

polymer/types/lib/mixins/element-mixin.d.ts

Line 141 in 9941451

$: {[key: string]: Element};

[TimvdLippe]

Hm. I actually think the intention is the web Element interface here. However, this name is already in scope and then TypeScript renames it. Cc @aomarks for thoughts?

…

On Tue, 30 Jan 2018, 22:04 Yasser Elsayed, ***@***.***> wrote: Now it's typed to return a Polymer.Element, which almost always not going to be the case. Why doesn't it just return an HtmlElement and let the user cast it if needed? https://github.com/Polymer/polymer/blob/99414511a5413c0e2d8ad7a494ba4ca015f6a534/types/lib/mixins/element-mixin.d.ts#L141 — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#5074>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFrDbyD8a24aOK6EepQubBeO9qZX4REJks5tP4P5gaJpZM4RzAjE> .

[kevinpschaaf]

As @TimvdLippe said, the intention is that the type is window.Element (since an id-mapped element could be a HTMLElement or SVGElement), and not Polymer.Element.

In the current 3.0-preview, modulizer converts the global Polymer.Element to Element as in (import Element from 'polymer-element.js'), however we intend to soon change modulizer to rename Polymer.Element to something other than Element to avoid confusion and actual scope-shadowing errors like this. We just need to decide on a better name.

[aomarks]

Just sent #5084 that should fix this issue for 2.x by defining a global _Element alias and updating all the references (the generator lets us do renames through the config file).

[yebrahim]

Thanks for the quick turnaround.

[floribon]

I just updated to Polymer 2.5 and this seems to create an issue with the TypeScript compiler as the global "Element" type is different from "HTMLElement" that was what I was getting from the $ function.

[dfreedm]

HTMLElement is a subclass of Element in the browser, so this may actually be correct. Ping @aomarks

[floribon]

It is, but previously we were getting this subclass HTMLElement so if it is the expected behavior that is a breaking change which should be stated in the release note. Current TS workaround is

const htmlElem = this.$.myId as HTMLElement;

[dfreedm]

Whoops, looks like the accidental shadowing of Element in typescript with Polymer.Element that was fixed in 2.5.0 was hiding the fact that this.$ has an incorrect typing. It should be a map of string to HTMLElement.
I'll file a bug.

[dfreedm]

Edit: HA, actually _this_ issue is exactly what you're hitting.

[yebrahim]

Yup. Any ideas when this will be out though? Or which version it's targeting?

[dfreedm]

@floribon Ok, after talking with the team, what you're seeing is working as intended.

Previously, the typing for Element would have actually been Polymer.Element which is a subclass of HTMLElement, and why your typing worked as written.

However, this was incorrect as you could have written code that expected all elements from this.$ to be instances of Polymer.Element, which could result in runtime errors, such as
let el = this.$.actuallyADiv.polymerSpecificAPI(), or if the element in this.$ was actually an SVGElement as @kevinpschaaf said here.

That said, I think we could be open to a new API to make Typescript integration a bit easier, such as the magic tagname typing that Element.prototype.querySelector('div') -> HTMLDivElement can provide.

[floribon]

@azakus thanks and sorry I missed the comment about the potential SVG match, it makes total sense. It may still be considered a potential breaking change or at least "meaningful change" even if it's a bug fix though.

[TimvdLippe]

I have opened a PR Polymer/gen-typescript-declarations#100 to generate these types automatically

[yebrahim]

I still think the returned type should be HTMLElement and not Element. Why make the assumption that the user will use this mostly for Polymer elements? I use this a lot to perform general DOM manipulations on child elements, and even if these elements are Polymer elements, I'll either need to reference their HTMLElement properties, or their own definitions, such as PaperInput for example. Both of these scenarios now require casting.

Let me know if you think this isn't the more common way of using the API, or if it's not what the API was meant for.

[TimvdLippe]

@yebrahim Note that with Element we mean https://developer.mozilla.org/en-US/docs/Web/API/Element, not Polymer.Element. E.g. HTMLElement is a subclass of Element and the list CAN therefore contain both HTMLElement and Polymer.Element as well as other classes such as https://developer.mozilla.org/en-US/docs/Web/API/SVGElement

[yebrahim]

Ah, that makes sense, my bad I didn't spot the difference. Thanks.