arqex / freezer

A tree data structure that emits events on updates, even if the modification is triggered by one of the leaves, making it easier to think in a reactive way.
MIT License
1.28k stars 56 forks source link

Make Freezer extendable (closes #22) #26

Closed kuraga closed 9 years ago

kuraga commented 9 years ago

1) We extend Freezer.prototype but not replace it. 2) We have to restore non-enumerable members of Freezer.prototype we need. The one is non-enumerable member of Freezer.prototype is Freezer.prototype.constructor.

marquex commented 9 years ago

Cool, this solution is simple and elegant! I will commit it later when I get out of work.

kuraga commented 9 years ago

@marquex @arqex , Note there is more elegant (all tests are green):

diff --git a/src/freezer.js b/src/freezer.js
index ed9a488..2c72cff 100644
--- a/src/freezer.js
+++ b/src/freezer.js
@@ -65,7 +65,7 @@ var Freezer = function( initialValue, mutable ) {
        this._events = [];
 }

-Freezer.prototype = Utils.createNonEnumerable({}, Emitter);
+Freezer.prototype.__proto__ = Emitter;
 //#build

 module.exports = Freezer;

But this way is not standard-compliant (precise: is not ES5-compliant, but it is ES6-compliant because __proto__ has been introduced in ES6).

Another side you use __proto__ here.

kuraga commented 9 years ago

See #27 for it

arqex commented 9 years ago

Hi @kuraga

Freezer should work in most browsers, and unfortunatelly __proto__ is not available in IE<11.

It is true that it is used by freezer to extend arrays, but there is a fallback for the browsers that don't support it. That's why I think that using __proto__ is not the best way of setting the constructor method.

I would do something like:

Freezer.prototype = Utils.createNonEnumerable({constructor: Freezer}, Emitter);
kuraga commented 9 years ago

@marquex @arqex This PR is like you say. Merge?

arqex commented 9 years ago

It doesn't work if the constructor is not enumerable?

kuraga commented 9 years ago

Ah, now I understand what do you mean. I just want to use Freezer.prototype as a Utils.createNonEnumerable's first argument but not {}. Freezer.prototype may be non-empty! What do you think?

kuraga commented 9 years ago

So I want something like

// object_assign is like Object.assign: it merges arguments in one object
var newPrototype = object_assign(Freezer.prototype, {constructor: Freezer});
Freezer.prototype = Utils.createNonEnumerable(newPrototype, Emitter);
arqex commented 9 years ago

Sure, createNonEnumerable creates an object with the non enumerable properties passed as the first parameter, and with the prototype passed as the second.

So a freezer prototype would be:

Freezer.prototype = Utils.createNonEnumerable({constructor: Freezer}, Emitter);

// Prototoype
Freezer.prototype; // { constructor: Freezer } 
Freezer.prototype.prototype: // Emitter

It should work with ES6, shouldn't it?

kuraga commented 9 years ago

I'm talking about one thing only: first argument of Utils.createNonEnumerable should contain Freezer.prototype (and, maybe, something more). Now (in master) it is {} and doesn't contain Freezer.prototype. I think it's incorrect.

kuraga commented 9 years ago

And in this line:

Freezer.prototype = Utils.createNonEnumerable({constructor: Freezer}, Emitter);

first argument ({constructor: Freezer}) doesn't contain Freezer.prototype. Isn't it bad?

arqex commented 9 years ago

There is nothing bad with that. In fact, until that line is executed Freezer.prototype is nothing but an empty object.

But we really don't care about what the prototype was because we are setting a new one, that is what Freezer.prototype = Utils.createNonEnumerable({}, Emitter); is doing now.

kuraga commented 9 years ago

Freezer.prototype is nothing but an empty object.

It is empty for me but is it always guaranteed to be empty?

kuraga commented 9 years ago

Another side, its enumerable part is guaranteed empty... Ok, sorry for my fault brainstorm.

One moment.

arqex commented 9 years ago

Sure, it is guaranteed. Freezer is a function defined just in the previous instruction.

kuraga commented 9 years ago

Close in favour of #29