sylvainpolletvillard / ObjectModel

Strong Dynamically Typed Object Modeling for JavaScript
http://objectmodel.js.org
MIT License
467 stars 30 forks source link

v2: Question on setConstructorProto() #103

Closed klues closed 5 years ago

klues commented 5 years ago

I have a question on v2: Why is this line needed: https://github.com/sylvainpolletvillard/ObjectModel/blob/2.x/dist/object-model.js#L126 ?

I use v2 because of IE11 compatibility and this line causes a weird bug in Chrome: https://bugs.chromium.org/p/chromium/issues/detail?id=919792 and I would be interested to understand this issue a little bit more...

sylvainpolletvillard commented 5 years ago

Hi, This line is required to get a valid constructor property for extended models or models derived from an ES6 class. See chapter 2.1 of this article for more details : https://dmitripavlutin.com/understanding-constructor-property/

This performance issue is solved in v3 by using a Proxy trap to specifically return a different value for the "constructor" property. But IE11 does not support Proxies so unfortunately you'll have to stick with v2.

klues commented 5 years ago

ok, thanks, but is this valid constructor actually needed? I've now removed the line in my project and everything still seems to work fine.

So do you have an explanation for my performance issue? Like described in https://bugs.chromium.org/p/chromium/issues/detail?id=919792 the lines

var test = [1,2,3];
test.constructor = function() {};

(which is actually also done in v2 of objectModel.js) cause that the completely independent encryption library I'm using to be 2x slower (or even more).

sylvainpolletvillard commented 5 years ago

It breaks several tests if this line is removed. If you don't use model extensions, it may be fine, although you could have some surprises if you need to use this constructor property one day.

The perf issue is definitely a bug, probably due to some specific V8 optimizations. Since you are not modifying Array.prototype in your tests, it should not have a perf penalty for any other native arrays. This is up to Chromium team now.

klues commented 5 years ago

ok, thanks for the info! I'll re-include the line once the Chrome Bug is fixed (hopefully).