varvet / serenade.js

Client side MVC framework
http://serenadejs.org
Other
524 stars 27 forks source link

Property not writable with Safari 5.1 #100

Closed antob closed 10 years ago

antob commented 10 years ago

Presuming Safari 5.1 is supported, i found the following odd behavour:

var obj = {};
Serenade.defineProperty(obj, "name");

obj.name = "My name";
obj.name // => "My name"

obj.name = "Your name";
obj.name // => "My name"

It seems the property is not writable. Adding property writable: true to the definition on line 52 in property.coffee seems to fix the problem.

I'm on Serenade revision 69fdcad6e2.

jnicklas commented 10 years ago

This is strange. writable is usually only relevant when Object.defineProperty is called with value, and not with get and set like Serenade does. It seems that the implementation of Object.defineProperty is incorrect in Safari 5.1. Seems like Serenade should support Safari 5.1, so adding the workaround is probably a good idea. What do you think?

antob commented 10 years ago

Unless you can think of any negative side effects, I'm all for it. At least it doesn't break any tests. Do you want me to do a PR?

jnicklas commented 10 years ago

Sure! Thanks. I don't think it will have negative side effects, it should do nothing in all other browsers.