Array index assignment does not work in Safari

[elliotchance]

In Safari 9.1 assigning an object to an array by index breaks anything after that:

https://jsfiddle.net/yfkkk42r/2/

Open the same URL in Chrome and it will work correctly.

[mweststrate]

Do you get an exception or something like that? (sorry, no IOS device at hand)

[hellectronic]

I tested on Safari 8.0.8 and got:

[Log] Remaining: Make coffee
[Log] Remaining: Spoil tea, Make coffee
[Log] Remaining: Make coffee
[Error] Failed to load resource: the server responded with a status of 404 (Not Found) https://fiddle.jshell.net/js/babel/foo.js.map

It looks like being a jsfiddle safari problem. It's correct on firefox.

[elliotchance]

I originally found the bug in our code, which means its not specifically related to JSFiddle.

You get no errors at all. When you assign an object like, the keys in that object are not observed so that if you change one later it never recognises the change.

Array.push works correctly, only on assignment to any index (from 0 to the length).

[mweststrate]

@elliotchance sorry for the late response! I'm back in town and just tried in on iOS9 device, indeed it doesn't run as expected. For others, to verify, just use this fiddle: https://jsfiddle.net/yfkkk42r/5/. If you don't see "iOS9 bug not present", the bug is present. Thanks for reporting, this is an important issue!

[andykog]

@mweststrate, the bug is also present on Desktop Safari 9.1.1

Seems, like Safari doesn't respect setting numbered setters on array-like constructor's prototype:

Object.defineProperty(ObservableArray.prototype, '9999', {
        enumerable: false,
        configurable: false,
        set: function() { alert('hello'); },
        get: function() { return 0; }
});
Object.defineProperty(ObservableArray.prototype, 'zzzz', {
        enumerable: false,
        configurable: false,
        set: function() { alert('hello'); },
        get: function() { return 0; }
});
var x = new ObservableArray(1);
x['zzzz'];     // ok, returns 0
x['zzzz'] = 5; // ok, alerts 'hello'
x['9999'];     // ok, returns 0
x['9999'] = 5; // fail, does nothing
x['9999'];     // fail, returns 5, property redefined

[andykog]

I'll do additional envestigations tomorrow

[mweststrate]

@andykog awesome! Thanks in advance

[andykog]

I have more precise example, that represents the problem:

function O() {}

var val;

Object.defineProperty(O.prototype, '1', {
  set: function() { val = '~~~ok~~~'; },
});

var x = new O();

x['1'] = 'something';

console.log(val); // undefined

I don't think there is a way to fix this behaviour. Safari won't use prototype's setter for number.

It is fixed in Safari 9.1.2 Technology Preview.

@mweststrate, I think, the only way to fix this issue is to detect if the bug is present in browser, and, if yes, to define getters/setters on instance, loosing performance. Is it ok?

[mweststrate]

@andykog did you try to use numbers as property keys? I think for MobX it actually doesn't matter much, not sure about that anymore whether the stringification was to satisfy the compiler or to fix something real. Otherwise falling back to non prototype properties if they don't work sounds like a very nice solution!

[andykog]

did you try to use numbers as property keys?

@mweststrate, it doesn't matter, using numbers (1) or string, that looks like number ('1') gives the same result. Basically, numbers are being stringified, when used as object keys.

[mweststrate]

@andykog If you need any help btw, or don't have the time for it, just let me know!

[mweststrate]

Fixed calls to getters and setters on super being called with wrong this object (https://developer.apple.com/safari/technology-preview/release-notes/, release 5, probably fixed the culprit)

[andykog]

I believe, I'll have time to dive in this tommorow.

By the way, did you ever tried to run tests in a real browser? I'm getting very strange results:
Chrome:

1..12061
reporter.js:30 # tests 12061
reporter.js:30 # pass  12057
reporter.js:30 # fail  4

Firefox:

1..12061
# tests 12061
# pass  12057
# fail  4

Safari:

[Log] 1..12058 (reporter.js, line 30)
[Log] # tests 12058 (reporter.js, line 30)
[Log] # pass  11995 (reporter.js, line 30)
[Log] # fail  63 (reporter.js, line 30)

Pushed e8a461c so you can try

[mweststrate]

Note: caching the property descriptors per index makes sure that at least the memory consumption doesn't increase significantly when defining them on the instance: 3635fa2

[andykog]

@mweststrate, are you going to merge 3635fa2 into master? Actually, this fixes Safari bug…

[mweststrate]

definitely! I'll try to publish it in a few hours

Op do 7 jul. 2016 19:07 schreef Andy Kogut notifications@github.com:

@mweststrate https://github.com/mweststrate, are you going to merge
3635fa2
3635fa2
into master? Actually, this fixes Safari bug…

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#364 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/ABvGhFzGhIRys1Zaq5mUBGxLVabQKdv9ks5qTTI_gaJpZM4I-zP3
.

[andykog]

You've accidentally found a good workaround! If you define at least 1 property that looks like number on instance, the bug disappears.

This doesnt work in safari:

const xPrototype = {};
Object.defineProperty(xPrototype, "1", {
    set: () => { alert("Got new value!") }
});
const x = Object.create(xPrototype);
x[1] = 1;

But this, actually does:

const xPrototype = {};
Object.defineProperty(xPrototype, "1", {
    set: () => { alert("Got new value!") }
});
const x = Object.create(xPrototype);
x[999999] = 5; // <- the fix
x[1] = 1;

[mweststrate]

Wow, that is really weird! Man, how did you even find that?! So your proposal is to simply fix this by assigning some value to this[-1] for example? Would that still work if the property is made non-enumerable after construction?

[andykog]

I just tested your branch 'enumerable-arrays' in safari, saw that it passes tests so I just found out why.
No, with your commit the instance properties are already being defined for non-empty initial values. We should only define non-enumerable configurable getter/setter for key '0' for empty array and the problem is solved.

On Jul 7, 2016, at 23:02, Michel Weststrate notifications@github.com wrote:

Wow, that is really weird! Man, how did you even find that?! So your proposal is to simply fix this by assigning some value to this[-1] for example? Would that still work if the property is made non-enumerable after construction?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[mweststrate]

Would you mind submitting a PR? I don't have a Mac so testing a patch would require me to distract my wife and then sneak away with her IPad 🎃

[andykog]

Of course!

On Jul 7, 2016, at 23:22, Michel Weststrate notifications@github.com wrote:

Would you mind submitting a PR? I don't have a Mac so testing a patch would require me to distract my wife and then sneak away with her IPad 🎃

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[andykog]

@mweststrate, there you go: #396

[mweststrate]

Released as 2.3.5. Thanks a lot!