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

[BUGFIX release] Don't setup mandatory setters on array indexes #18741

Merged
merged 1 commit into from
Feb 28, 2020

Conversation

pzuraq
Copy link
Contributor

@pzuraq pzuraq commented Feb 12, 2020

It turns out that removing items from an array (e.g. via splice) will
also remove their descriptors, if any. Could be a browser bug, but I'm
not sure we intended to add setters directly to arrays in the first
place, and if we can't definitely manage them we probably shouldn't.

Fixes #18739

@pzuraq pzuraq changed the title [BUGFIX] Don't setup mandatory setters on array indexes [BUGFIX release] Don't setup mandatory setters on array indexes Feb 18, 2020
@@ -34,6 +34,12 @@ if (DEBUG) {

SEEN_TAGS!.add(tag);

if (Array.isArray(obj) && !isNaN(Number(keyName))) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After discussion with @krisselden, he suggested we change to using something more like:

function isElementKey(key) {
    return typeof key === 'number' ? isPositiveInt(key) : isStringInt(key);
}

function isStringInt(str) {
    let num = parseInt(str, 10);
    return isPositiveInt(num) && str === '' + num;
}

function isPositiveInt(num) {
    return num >= 0 && num % 1 === 0;
}

// this would replace the conditional
if (isElementKey(keyName)) {
  return;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pzuraq ping 😺

@pzuraq pzuraq force-pushed the bugfix/mandatory-setters-dont-clobber branch from 6d17631 to 279217c Compare February 27, 2020 22:04
It turns out that removing items from an array (e.g. via `splice`) will
also remove their descriptors, if any. Could be a browser bug, but I'm
not sure we intended to add setters directly to arrays in the first
place, and if we can't definitely manage them we probably shouldn't.
@pzuraq pzuraq force-pushed the bugfix/mandatory-setters-dont-clobber branch from 279217c to 5aa4c61 Compare February 28, 2020 00:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[3.16] Bug with set and array
2 participants