Core: Use Array#flat where supported #4459

aelafifi · 2019-08-19T08:14:02Z

Summary

Solve $.map() issue when working with large arrays on Chrome browser (#4320)

Checklist

All authors have signed the CLA at https://cla.js.foundation/jquery/jquery
New tests have been added to show the fix or feature works
Grunt build and unit tests pass locally with these changes
If needed, a docs issue/PR was created at https://github.com/jquery/api.jquery.com

jsf-clabot · 2019-08-19T08:14:05Z

All committers have signed the CLA.

aelafifi · 2019-08-19T08:24:18Z

I've used the native .flat() function wherever it's supported.
Otherwise, .concat.apply() will be called, because after trying implementing a custom logic to flat the array, it takes a very long time to handle it in the browser, and as mentioned @ Won't Fix:

Serious performance implications of fixing a relatively rare problem

Other solution will be good (from the functional side) is to put the flatting logic into the main loop in $.map() itself, but I think it's a bad design, especially in this case since it's very rare!

src/var/flat.js

mgol · 2019-08-19T09:50:25Z

Thanks for the PR! See my comment & please sign our CLA.

src/var/flat.js

To avoid massive indentation changes when migrating to ES modules.

src/var/flat.js

mgol · 2019-08-19T11:38:09Z

Also, it'd be great if the PR title & commit messages followed the format from https://contribute.jquery.org/commits-and-pull-requests/#commit-guidelines

We have a pre-commit hook that enforces that & it should catch those transgressions, do you have Node.js installed locally?

aelafifi · 2019-08-19T11:53:51Z

Also, it'd be great if the PR title & commit messages followed the format from https://contribute.jquery.org/commits-and-pull-requests/#commit-guidelines

Thanks, I'll check it.

We have a pre-commit hook that enforces that & it should catch those transgressions, do you have Node.js installed locally?

Yes, I've Node.js v12.6.0

mgol · 2019-08-19T12:02:58Z

@aelafifi Can you make sure all usages of the concat.apply( [], something ) pattern are converted? I see at least one other usage in manipulation.

src/core.js

Co-Authored-By: Michał Gołębiowski-Owczarek <m.goleb@gmail.com>

With simplified call for the function

mgol

Size increase at this point: +21 bytes. Looks fine to me, I added one comment in tests.

test/unit/core.js

src/var/flat.js

mgol · 2019-08-19T15:20:18Z

I ran some benchmarks: flat is the fastest in Chrome, Firefox & Safari: https://jsperf.com/flat-vs-concat-vs-iteration

aelafifi · 2019-08-19T15:43:17Z

I'll work on them as soon as possible,
I have a question about the PR name, why it's related to component Selector?

timmywil · 2019-08-19T15:56:30Z

@aelafifi You're right that it's more universal than selector, but the main use for this historically has been to array-ify the result of selections.

mgol · 2019-08-19T15:58:20Z

@aelafifi Sorry, my mistake, it should have been Core as it's used in other places as well, e.g. manipulation.

mgol · 2019-08-22T10:16:53Z

I'll remove the "Needs review" label for now as the PR needs some further work.

aelafifi · 2019-09-10T09:38:14Z

Sorry for being late.
I've updated the points above, please check them and tell me if there is any thing wrong.

timmywil

Seems good. Can we get rid of the concat var file?

aelafifi · 2019-09-12T15:44:02Z

It still used from the flat var file as a secondary option if Array#flat isn't exists

timmywil · 2019-09-13T13:14:13Z

That’s not what I meant. We’re not using concat anywhere else so it doesn’t need to be its own var file.

aelafifi · 2019-09-18T09:19:18Z

Great, it's done.

mgol

One comment to add, otherwise looks good to me.

test/unit/core.js

timmywil

LGTM

Calling `Array.prototype.concat.apply( [], inputArray )` to flatten `inputArray` crashes for large arrays; using `Array.prototype.flat` avoids these issues in browsers that support it. In case it's necessary to support these large arrays even in older browsers, a polyfill for `Array.prototype.flat` can be loaded. This is already being done by many applications. (cherry picked from 9df4f1d) Fixes jquerygh-4320 Closes jquerygh-4459

mgol · 2019-09-25T17:54:53Z

Landed on master (9df4f1d) & 3.x-stable (2f666c1), changes will be in jQuery 3.5.0. Thanks!

aelafifi added 3 commits August 19, 2019 10:04

Add support to native .flat() function from array

b042b7a

Add unit tests

67ca9b0

Merge branch 'master' into support_native_array_flat

78ae81b

mgol requested changes Aug 19, 2019

View reviewed changes

src/var/flat.js Outdated Show resolved Hide resolved

mgol reviewed Aug 19, 2019

View reviewed changes

src/var/flat.js Outdated Show resolved Hide resolved

aelafifi added 2 commits August 19, 2019 13:14

Remove indent

94ce632

To avoid massive indentation changes when migrating to ES modules.

Add support comment

6e4a28f

mgol reviewed Aug 19, 2019

View reviewed changes

src/var/flat.js Outdated Show resolved Hide resolved

mgol reviewed Aug 19, 2019

View reviewed changes

src/var/flat.js Outdated Show resolved Hide resolved

mgol added the Core label Aug 19, 2019

mgol mentioned this pull request Aug 19, 2019

$.map() fails with large arrays in Сhrome #4320

Closed

mgol reviewed Aug 19, 2019

View reviewed changes

src/core.js Outdated Show resolved Hide resolved

Ahmed S. El-Afifi and others added 4 commits August 19, 2019 14:38

Update the comment phrase

7cfd435

Co-Authored-By: Michał Gołębiowski-Owczarek <m.goleb@gmail.com>

Core: use flat instead of concat for all occurrences

507a15c

With simplified call for the function

Modify support browser versions

ad5d01c

Hotfix: call flat directly without .call

039ce10

mgol requested changes Aug 19, 2019

View reviewed changes

test/unit/core.js Outdated Show resolved Hide resolved

mgol changed the title ~~Support native array .flat() function instead of .concat.apply()~~ Selector: Use Array#flat where supported Aug 19, 2019

mgol reviewed Aug 19, 2019

View reviewed changes

src/var/flat.js Outdated Show resolved Hide resolved

timmywil added the Needs review label Aug 19, 2019

mgol changed the title ~~Selector: Use Array#flat where supported~~ Core: Use Array#flat where supported Aug 19, 2019

mgol mentioned this pull request Aug 19, 2019

[select] use for loop instead of push.apply jquery/sizzle#403

Open

mgol removed the Needs review label Aug 22, 2019

aelafifi added 3 commits September 10, 2019 11:27

Core: remove hardcoded default value for Array#flat

2f7baf8

Core: remove hardcoded default value for Array#flat

dea751a

Core: add guard for tests which requires Array#flat existence

d95cacf

timmywil approved these changes Sep 10, 2019

View reviewed changes

Core: remove concat var file

5bf3b7a

mgol requested changes Sep 21, 2019

View reviewed changes

test/unit/core.js Show resolved Hide resolved

mgol added the Needs review label Sep 21, 2019

Core: add browser support comments for test guard

6bf0abb

timmywil approved these changes Sep 23, 2019

View reviewed changes

mgol approved these changes Sep 23, 2019

View reviewed changes

mgol removed the Needs review label Sep 24, 2019

mgol modified the milestones: 4.0.0, 3.5.0 Sep 24, 2019

mgol closed this in 9df4f1d Sep 25, 2019

lock bot locked as resolved and limited conversation to collaborators Mar 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core: Use Array#flat where supported #4459

Core: Use Array#flat where supported #4459

aelafifi commented Aug 19, 2019 •

edited

jsf-clabot commented Aug 19, 2019 •

edited

aelafifi commented Aug 19, 2019

mgol commented Aug 19, 2019

mgol commented Aug 19, 2019

aelafifi commented Aug 19, 2019

mgol commented Aug 19, 2019

mgol left a comment

mgol commented Aug 19, 2019 •

edited

aelafifi commented Aug 19, 2019

timmywil commented Aug 19, 2019

mgol commented Aug 19, 2019

mgol commented Aug 22, 2019

aelafifi commented Sep 10, 2019

timmywil left a comment

aelafifi commented Sep 12, 2019

timmywil commented Sep 13, 2019

aelafifi commented Sep 18, 2019

mgol left a comment

timmywil left a comment

mgol commented Sep 25, 2019 •

edited

Core: Use Array#flat where supported #4459

Core: Use Array#flat where supported #4459

Conversation

aelafifi commented Aug 19, 2019 • edited

Summary

Checklist

jsf-clabot commented Aug 19, 2019 • edited

aelafifi commented Aug 19, 2019

mgol commented Aug 19, 2019

mgol commented Aug 19, 2019

aelafifi commented Aug 19, 2019

mgol commented Aug 19, 2019

mgol left a comment

Choose a reason for hiding this comment

mgol commented Aug 19, 2019 • edited

aelafifi commented Aug 19, 2019

timmywil commented Aug 19, 2019

mgol commented Aug 19, 2019

mgol commented Aug 22, 2019

aelafifi commented Sep 10, 2019

timmywil left a comment

Choose a reason for hiding this comment

aelafifi commented Sep 12, 2019

timmywil commented Sep 13, 2019

aelafifi commented Sep 18, 2019

mgol left a comment

Choose a reason for hiding this comment

timmywil left a comment

Choose a reason for hiding this comment

mgol commented Sep 25, 2019 • edited

aelafifi commented Aug 19, 2019 •

edited

jsf-clabot commented Aug 19, 2019 •

edited

mgol commented Aug 19, 2019 •

edited

mgol commented Sep 25, 2019 •

edited