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

Make Freezer extendable (closes #22) #26

Closed
wants to merge 1 commit into from
Closed

Make Freezer extendable (closes #22) #26

wants to merge 1 commit into from

Conversation

kuraga
Copy link
Contributor

@kuraga kuraga commented Apr 30, 2015

  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.

1) We extend `Freezer.prototype` but not replace it.
2) We have restore non-enumerable members of `Freezer.prototype` we need.
The of non-enumerable member of `Freezer.prototype` is `Freezer.prototype.constructor`.
This was referenced Apr 30, 2015
@marquex
Copy link

marquex commented Apr 30, 2015

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

@kuraga
Copy link
Contributor Author

kuraga commented Apr 30, 2015

@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
Copy link
Contributor Author

kuraga commented Apr 30, 2015

See #27 for it

@arqex
Copy link
Owner

arqex commented May 1, 2015

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 kuraga mentioned this pull request May 1, 2015
@kuraga
Copy link
Contributor Author

kuraga commented May 1, 2015

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

@arqex
Copy link
Owner

arqex commented May 1, 2015

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

@kuraga
Copy link
Contributor Author

kuraga commented May 1, 2015

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
Copy link
Contributor Author

kuraga commented May 1, 2015

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
Copy link
Owner

arqex commented May 1, 2015

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
Copy link
Contributor Author

kuraga commented May 1, 2015

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
Copy link
Contributor Author

kuraga commented May 1, 2015

And in this line:

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

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

@@ -65,7 +65,9 @@ var Freezer = function( initialValue, mutable ) {
this._events = [];
}

Freezer.prototype = Utils.createNonEnumerable({}, Emitter);
Freezer.prototype = Utils.createNonEnumerable(Freezer.prototype, Emitter);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you see what I changed in THIS (68) line? That's I'm talking about.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything work without this change! But I think this change is right!!!

@arqex
Copy link
Owner

arqex commented May 1, 2015

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
Copy link
Contributor Author

kuraga commented May 1, 2015

Freezer.prototype is nothing but an empty object.

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

@kuraga
Copy link
Contributor Author

kuraga commented May 1, 2015

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

One moment.

@arqex
Copy link
Owner

arqex commented May 1, 2015

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

@kuraga
Copy link
Contributor Author

kuraga commented May 1, 2015

Close in favour of #29

@kuraga kuraga closed this May 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants