Skip to content
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

Cannot extend HTMLElement on IE11 with dcl 1.1.3 #18

Closed
EAlexRojas opened this issue Apr 14, 2015 · 12 comments
Closed

Cannot extend HTMLElement on IE11 with dcl 1.1.3 #18

EAlexRojas opened this issue Apr 14, 2015 · 12 comments
Assignees
Labels
Milestone

Comments

@EAlexRojas
Copy link

On the delite/deliteful IBM's projects we are using dcl to extend HTMLElement and create custom elements.
Until version 1.1.2 it works good, but with the change introduced on version 1.1.3 (on this commit)
It does not work anymore.

I think that validation is because usually what we think of as "classes" are actually (technically) javascript functions, but we are extending HTMLElement and that typeof HTMLElement returns "function" on chrome and firefox, but at least on Internet Explorer it returns "object".
So something as simple as dcl(HTMLElement, {}) will fail.

What do you think about it ?

@uhop
Copy link
Owner

uhop commented Apr 14, 2015

Thank you for the bug request with the repro case. Is it specific to a certain browser or can be demonstrated on all of them?

UPDATE: I see it fails on IE11. I believe it was already reported, but I'll double-check it.

@edchat
Copy link

edchat commented Apr 14, 2015

It also fails on safari.

@uhop
Copy link
Owner

uhop commented Apr 14, 2015

OK, in this case we need a comprehensive solution. :-( I'll see what I can do.

@wkeese
Copy link

wkeese commented Apr 21, 2015

To be clear, the regression is just from b4b492c, so the easiest thing would be to rollback that change.

You could also probably change the check to be something more permissive, like typeof f === "function" || f instanceof HTMLElement (except you need a little more code so that doesn't fail on Node, where HTMLElement isn't defined).

@uhop
Copy link
Owner

uhop commented Apr 21, 2015

I don't want to special-case HTMLElement, because I am 100% sure there are others. Interesting to see that apparently HTMLElement is not a function — how standard is it?

There are several problems here:

  1. A constructor does not allow to attach any properties.
    1. Setting a property is silently ignored.
    2. Setting a property raises an exception.
  2. Due to (1) above we cannot "mark" an object and associate an additional information with it.

Until we have maps of ES6 we cannot associate data with abstract immutable objects. One possible hack is to use an output of toString() as a key for the association. It is a hack, because there is no guarantee that the output is unique.

In any case adding an additional way to associate arbitrary data complicates everything. That's why I am taking my time to resolve the issue generally rather than specifically for HTMLElement.

@wkeese
Copy link

wkeese commented Apr 21, 2015

You can solve all those problems by rolling back b4b492c.

Also, I have no idea why you are talking about attaching properties to a constructor. AFAICT that wouldn't help solve this issue at all.

@uhop
Copy link
Owner

uhop commented Apr 21, 2015

The code in question adds a property ctr to a method (a function). It can be a constructor — it doesn't really matter. That is why I am talking about "attaching properties to a constructor". Adding a property ctr (as in the patch you referenced twice already) is required for this.inherited() to work. It doesn't make any sense to add it to non-functional properties. Hence the added test. The whole patch was done after a bug report, which involved setting it on a primitive (silently ignored), while in strict mode (violently throws).

In general dcl associates an external data with objects in two logical ways:

  • Adds a unique id property to every base, if it is missing. It affects primordial constructors.
  • Adds a constructor property to a method, so it can retrieve a meta information (if any) later in this.inherited(). It may affect non-functional properties, if they happened to be in a chain of normal methods.

Obviously both of them may fail on immutable objects, and the only sure way to deal with them are ES6-style maps, which are years away. Or invent a way to remove the necessity of modifying them at all.

@wkeese
Copy link

wkeese commented Apr 22, 2015

OK I see now why you are setting f.ctor, although it's disappointing for us since we aren't even using this.inherited(). It's unfortunate that code is in mini.js, rather than inherited.js.

@uhop
Copy link
Owner

uhop commented Apr 22, 2015

It should be refactored. In general I think dcl should be more conservative adding properties, even harmless properties. Not only this.inherited() is not used in your particular case, but it cannot be used from a primordial object ⇒ no need to set it ever. It looks like 1.2 will be coming soon to address this issue in an ordered fashion.

@uhop uhop modified the milestones: 1.2, Future Apr 22, 2015
wkeese added a commit to ibm-js/delite that referenced this issue Apr 22, 2015
asudoh added a commit to asudoh/liaison that referenced this issue Apr 27, 2015
asudoh added a commit to asudoh/liaison that referenced this issue Apr 27, 2015
@uhop uhop added the ready label Apr 30, 2015
@cjolif
Copy link

cjolif commented Jul 13, 2015

@uhop any news?

@uhop
Copy link
Owner

uhop commented Jul 16, 2015

@cjolif Struggling with performance tests. V8 does not optimize functions that use get/set. See https://github.com/petkaantonov/bluebird/wiki/Optimization-killers

@uhop uhop modified the milestones: 2.0, 1.2 Jun 10, 2017
@uhop
Copy link
Owner

uhop commented Jun 10, 2017

Addressed in 2.0.

@uhop uhop closed this as completed Jun 10, 2017
@uhop uhop removed the ready label Jun 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants