Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

hubgit
Copy link

@hubgit hubgit commented Dec 2, 2023

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.

image

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:

image

Safari 17.1:

image

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
Copy link
Author

hubgit commented Dec 2, 2023

Chrome 120.0.6099.56:

image

Node 20.7.0:

image

@zloirock
Copy link
Owner

zloirock commented Dec 3, 2023

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
Copy link
Author

hubgit commented Dec 3, 2023

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
Copy link
Owner

zloirock commented Dec 3, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants