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: Tests for asMutable fail on some platforms #110

Closed tad-lispy closed 8 years ago

tad-lispy commented 8 years ago

Fix two tests that are failing on some platforms.

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

Platforms

  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

Errors

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.

Reason

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.

Solution

Test if type of value passed to asDeepMutable is object and only recur if it is. Otherwise return unchanged value. This shall prevent calling asMutable with deep option on leaf properties of immutable object.

Also the if statement is now split into multiple lines for readability - there are four alternative conditions there and it was looking really messy on a single line IMO.

rtfeldman commented 8 years ago

Looks great! Thanks for the level of detail on this. :heart_eyes_cat:

tad-lispy commented 8 years ago

You are welcome and thanks for Seamless. It is so much fun to work with.

rtfeldman commented 8 years ago

@lzrski published as 5.1.1!