Push an empty string when testing whether [].length is writable

zloirock / core-js

Standard Library

MIT License

24.61k stars 1.66k forks source link

Push an empty string when testing whether [].length is writable #1314

Closed hubgit closed 12 months ago

hubgit commented 1 year ago

It seems that the current version of Chrome (v119.0.6045.199, on macOS 14.1.1) is not throwing the expected error when calling push with no arguments.

This leads to Array.prototype.push being polyfilled more than expected, affecting performance.

Firefox and Safari throw the expected error both with and without this change, although the message is slightly different in Firefox when pushing an empty string.

Firefox 120.0.1:

Safari 17.1:

I haven't tested with any other browsers or versions.

It's possible that this does actually indicate a case where Array.prototype.push needs to be polyfilled, of course, in which case feel free to close this PR.

hubgit commented 1 year ago

Chrome 120.0.6099.56:

Node 20.7.0:

zloirock commented 12 months ago

It should be thrown even without arguments. It's a known V8 bug and should be fixed on the V8 side. When I added this fix, I thought it would be fixed in the next Chrome release, however, they are in no hurry to fix it. You can remind them about this bug.

hubgit commented 12 months ago

Ok, thanks - I'll continue to exclude the polyfill locally by adding exclude: ['es.array.push'] to the config for @babel/preset-env.

A few thoughts:

The comment talks about the bug being that browsers throw InternalError rather than TypeError, but it seems that this is testing something different: that v8 actually throws an error when push() is called with no arguments on an array where length is not writable.
It seems, perhaps, reasonable that an array's length is not written to when push() is called with no arguments, as the array's length hasn't changed.
I'm not sure when push() would be called in real life with no arguments, which means I'm unsure whether it needs to be polyfilled.
The performance impact of the polyfill in Chrome can be substantial in applications where push is used frequently.

zloirock commented 12 months ago

InternalError is an issue in ancient FF and thrown for another reason, it's just a note.

It seems, perhaps, reasonable that an array's length is not written to when push() is called with no arguments, as the array's length hasn't changed.

Step 6.

The performance impact of the polyfill in Chrome can be substantial in applications where push is used frequently.

It is the reason why it should be fixed on the V8 side ASAP - but, apparently, they do not care about the performance of real applications, which usually contain core-js. It was added in the scope of a set of different bug fixes and it's not a reason to remove bug fixes from core-js just because some engines don't wanna fix those bugs.