uhop / dcl

Elegant minimalistic implementation of OOP with mixins + AOP in JavaScript for node.js and browsers.
http://www.dcljs.org/
Other
76 stars 10 forks source link

Cannot extend HTMLElement on IE11 with dcl 1.1.3 #18

Closed EAlexRojas closed 7 years ago

EAlexRojas commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

It also fails on safari.

uhop commented 9 years ago

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

wkeese commented 9 years ago

To be clear, the regression is just from b4b492c2f59aaef2dfec5830107cf0761fc484c1, 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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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:

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 commented 9 years ago

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 commented 9 years ago

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.

cjolif commented 9 years ago

@uhop any news?

uhop commented 9 years ago

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

uhop commented 7 years ago

Addressed in 2.0.