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

Fix __spreadArray for non-concat-spreadables #45386

Merged
merged 1 commit into from Aug 10, 2021

Conversation

rbuckton
Copy link
Member

@rbuckton rbuckton commented Aug 9, 2021

The __spreadArray helper uses Array.concat, which doesn't work with DOM NodeList or HTMLCollection. This changes the __spreadArray helper to fall back to Array.prototype.slice.call(from) to ensure the elements of from are concatenated properly.

Fixes TypeScript side of microsoft/tslib#153

@@ -647,7 +647,7 @@ namespace ts {
ar[i] = from[i];
}
}
return to.concat(ar || from);
return to.concat(ar || Array.prototype.slice.call(from));
Copy link
Member

Choose a reason for hiding this comment

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

I don't know how crazy we want to get with this one but we could also do

Suggested change
return to.concat(ar || Array.prototype.slice.call(from));
return to.concat(ar || (from instanceof Array ? from : Array.prototype.slice.call(from)));

Copy link
Member Author

Choose a reason for hiding this comment

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

I considered that, including a check for isConcatSpreadable, but it just became long-winded and the performance benefits seemed negligible.

Copy link
Member Author

@rbuckton rbuckton Aug 10, 2021

Choose a reason for hiding this comment

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

I was originally thinking of doing this:

var __spreadArray = (this && this.__spreadArray) || (function () {
    var toString = Object.prototype.toString;
    var slice = Array.prototype.slice;
    var isArray = typeof Array.isArray === "function" ? Array.isArray : function (obj) { return toString.call(obj) === "[object Array]"; };
    var isConcatSpreadable = typeof Symbol === "function" && Symbol.isConcatSpreadable;
    function canConcat(obj) { return isArray(obj) || isConcatSpreadable && obj[isConcatSpreadable]; }
    return function (to, from, pack) {
        if (pack || arguments.length === 2) {
            for (var i = 0, l = from.length, ar; i < l; i++) {
                if (ar || !(i in from)) {
                    if (!ar) ar = slice.call(from, 0, i);
                    ar[i] = from[i];
                }
            }
            if (ar) from = ar;
        }
        return to.concat(canConcat(from) ? from : slice.call(from));
    }
})();

@DanielRosenwasser
Copy link
Member

We'll need to cherry-pick this in for release-4.4, so ping me when you're ready to merge.

@rbuckton rbuckton merged commit d8e830d into main Aug 10, 2021
@rbuckton rbuckton deleted the fixSpreadArrayForNonConcatSpreadables branch August 10, 2021 00:08
@rbuckton
Copy link
Member Author

Also, take a look at microsoft/tslib#155 to fix this on the tslib side, since the concat-spreadable thing is a back-compat issue.

@DanielRosenwasser
Copy link
Member

@typescript-bot cherry-pick this to release-4.4

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 11, 2021

Heya @DanielRosenwasser, I've started to run the task to cherry-pick this into release-4.4 on this PR at 7c7e0eb. You can monitor the build here.

@DanielRosenwasser
Copy link
Member

@typescript-bot cherry-pick this to release-4.4 and LKG

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 11, 2021

Heya @DanielRosenwasser, I've started to run the task to cherry-pick this into release-4.4 on this PR at 7c7e0eb. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

Hey @DanielRosenwasser, I've opened #45416 for you.

typescript-bot pushed a commit to typescript-bot/TypeScript that referenced this pull request Aug 11, 2021
Component commits:
7c7e0eb Fix __spreadArray for non-concat-spreadables
@typescript-bot
Copy link
Collaborator

Hey @DanielRosenwasser, I couldn't open a PR with the cherry-pick. (You can check the log here). You may need to squash and pick this PR into release-4.4 manually.

typescript-bot pushed a commit to typescript-bot/TypeScript that referenced this pull request Aug 11, 2021
Component commits:
7c7e0eb Fix __spreadArray for non-concat-spreadables
BobobUnicorn pushed a commit to BobobUnicorn/TypeScript that referenced this pull request Oct 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants