search

rtfeldman / seamless-immutable

Immutable data structures for JavaScript which are backwards-compatible with normal JS Arrays and Objects.
BSD 3-Clause "New" or "Revised" License
5.37k stars 195 forks source link

FIX #76 Incompatible with Object.create(null) #104

Closed tad-lispy closed 8 years ago

tad-lispy commented 8 years ago

Changed all invocations of <obj>.hasOwnProperty(<key>) to Object.hasOwnProperty.call(<obj>, <key>), unless I was certain that is instance of Immutable or Array.

Unfortunately I did not provide any test cases for the compatibility with prototypeless objects. Any help here would be appreciated. Sorry, but the setup of test suit is quite sophisticated and I had only limited time to work on that.

I have tested it manually and it seems to work. Lack of real unit tests is the reason for question mark in commit message title.

All existing tests are passing in my local environment.

76

tad-lispy commented 8 years ago

The CI build is still failing, this time when trying to run Zuul tests. Is there anything I can do about it?

rtfeldman commented 8 years ago

Hm, this would be a performance regression given that call and apply are significantly slower than direct invocation.

Was there a reason the Object.getOwnPropertyDescriptor approach discussed in #76 wouldn't work?

tad-lispy commented 8 years ago

@rtfeldman no, not really. I guess I was too lazy to read my own issue.

Should be better now.

rtfeldman commented 8 years ago

@lzrski hm, looks like build fails on Android: https://travis-ci.org/rtfeldman/seamless-immutable

tad-lispy commented 8 years ago

I did some research and actually the complete list of platforms that are failing is:

  1. android 4.0 on Linux
  2. android 5.1 on Linux
  3. internet explorer 10 on Windows 2012
  4. internet explorer 11 on Windows 10
  5. internet explorer 9 on Windows 2008
  6. opera 12 on Windows 2003
  7. safari 6 on Mac 10.8

They all fail at the same two tests:

  1. ImmutableArray :: #asMutable :: returns a deep mutable copy if provided the deep flag
  2. ImmutableObject :: #asMutable :: returns a deep mutable copy if provided the deep flag

The errors are:

Platform Error
IE Object.getOwnPropertyDescriptor: argument is not an Object
Safari Requested property descriptor of a value that is not an object.
Opera Object.getOwnPropertyDescriptor: first argument not an Object
Android Object.getOwnPropertyDescriptor called on non-object

Which practically boils down to the same thing.

Why

According to ES 5.1 standard if first argument to Object.getOwnPropertyDescriptor is not an Object it throws a TypeError exception. That's what we get in failing platforms.

According to ES 2015 standard the first argument is always coerced to object.

All failing platforms seem to behave according to ES 5 standard. In our case the function causing the errors is asDeepMutable, which just passes whatever it gets to Object.getOwnPropertyDescriptor. It is being called by two other functions asMutableArray and asMutableObject. Both iterate indiscriminately over elements / keys and pass them to asDeepMutable. So when the value is not an object (e.g. a number, null, etc.) the ES 5 platforms are throwing errors.

The solution is probably to check the type of argument passed to asDeepMutable.

PR comming soon :)