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(animations): replace copy of query selector node-list from "spread" to "for" #39646

Closed
wants to merge 1 commit into from

Conversation

basherr
Copy link
Contributor

@basherr basherr commented Nov 12, 2020

The animation driver fails to query a large number of query selectors. Limitation varies across all browsers.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: #38551

What is the new behavior?

Spread syntax(...) exceeds stake size in browsers. The following workbench demonstrates the limitation both in chrome and firefox:

https://jsbench.me/r1khehfd20/1

As per @AleksanderBodurri workbench example, the performance increases slightly with for loop compared to the rest:

https://jsbench.me/grkhenmtkb/1

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Fixes #38551

@google-cla google-cla bot added the cla: yes label Nov 12, 2020
@pullapprove pullapprove bot requested a review from jelbourn Nov 12, 2020
@basherr basherr changed the title fix animations: replace copy of query selector node list from "spread(...)" to "for" to avoid callstack error fix(animations): replace copy of query selector node list from "spread(...)" to "for" to avoid callstack error Nov 12, 2020
@basherr basherr changed the title fix(animations): replace copy of query selector node list from "spread(...)" to "for" to avoid callstack error fix(animations): replace copy of query selector node-list from "spread" to "for" Nov 12, 2020
@ngbot ngbot bot added this to the needsTriage milestone Nov 12, 2020
@petebacondarwin petebacondarwin added action: review P4 Low-priority issue that needs to be resolved refactoring Issue that involves refactoring or code-cleanup target: patch This PR is targeted for the next patch release labels Nov 12, 2020
@basherr
Copy link
Contributor Author

@basherr basherr commented Nov 13, 2020

@petebacondarwin Would you please just let me know why are circle ci tests failing?

@petebacondarwin
Copy link
Member

@petebacondarwin petebacondarwin commented Nov 13, 2020

@basherr - it looks like the CI is being run under your account rather than Angular's. Did you turn on CircleCI integration for your own fork? I suggest turning that off since it might be blocking the normal Angular CI running?

@basherr
Copy link
Contributor Author

@basherr basherr commented Nov 14, 2020

@basherr - it looks like the CI is being run under your account rather than Angular's. Did you turn on CircleCI integration for your own fork? I suggest turning that off since it might be blocking the normal Angular CI running?

Is there a way to re-run the tests? Just turned off CircleCi on my fork. Thanks

@JoostK
Copy link
Member

@JoostK JoostK commented Nov 14, 2020

@basherr - it looks like the CI is being run under your account rather than Angular's. Did you turn on CircleCI integration for your own fork? I suggest turning that off since it might be blocking the normal Angular CI running?

Is there a way to re-run the tests? Just turned off CircleCi on my fork. Thanks

The CI run is failing because the commits need to be rebased on top of latest master. The instructions are provided in the CI job:

        CircleCI config on master has been modified since commit 6d13473,
        which this PR is based on.

        Please rebase the PR on master after fetching from upstream.

        Rebase instructions for PR Author, please run the following commands:

          git fetch upstream master;
          git checkout fix-animation-driver-query;
          git rebase upstream/master;
          git push --force-with-lease;

In this case, you'll also want to squash all commits into a single one, so you'll want to run an interactive rebase and mark all commits but the first as squash.

@google-cla
Copy link

@google-cla google-cla bot commented Nov 14, 2020

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no and removed cla: yes labels Nov 14, 2020
@basherr basherr force-pushed the fix-animation-driver-query branch from 5c6f825 to 348a0db Compare Nov 14, 2020
@google-cla google-cla bot added cla: yes and removed cla: no labels Nov 14, 2020
@basherr basherr force-pushed the fix-animation-driver-query branch from e7390eb to 53c9e9e Compare Nov 14, 2020
@AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Nov 25, 2020

Hi @basherr,

Could you please squash all commits in this PR and update commit message (git commit --amend)?

Here is my proposal (based on the comments in the code):

fix(animations): replace copy of query selector node-list from "spread" to "for"

For element queries that return sufficiently large NodeList objects, using spread syntax
to populate the results array causes a RangeError due to the call stack limit being reached.
This commit updates the code to use regular "for" loop instead.

Fixes #38551.

Thank you.

@basherr basherr force-pushed the fix-animation-driver-query branch 2 times, most recently from 6f84b7c to 970cff1 Compare Nov 25, 2020
@basherr
Copy link
Contributor Author

@basherr basherr commented Nov 25, 2020

@AndrewKushnir The build fails due to the commit message body being too long. What about the following only?

fix(animations): replace copy of query selector node-list from "spread" to "for"

The spread syntax fails to copy a sufficiently large number of Nodelist due to callstack limit being reached. 

@petebacondarwin
Copy link
Member

@petebacondarwin petebacondarwin commented Nov 25, 2020

@basherr - it is only the length of "each line" that must be under 120 chars I suspect that you put the whole body in a single line? Try splitting the body into multiple lines like in @AndrewKushnir's example.

@petebacondarwin
Copy link
Member

@petebacondarwin petebacondarwin commented Nov 25, 2020

Also there are still 2 commits in this PR. Can you squash them into one?

…d" to "for"

For element queries that return sufficiently large NodeList
objects, using spread syntax to populate the results array
causes a RangeError due to the call stack limit being reached.
This commit updates the code to use regular "for" loop instead.

Fixes angular#38551.
@basherr basherr force-pushed the fix-animation-driver-query branch from 970cff1 to ecab8ac Compare Nov 25, 2020
@basherr
Copy link
Contributor Author

@basherr basherr commented Nov 25, 2020

@AndrewKushnir @petebacondarwin All set! thanks for your help. However, the reviewers seem too much occupied.

@petebacondarwin petebacondarwin added the action: presubmit A standard presubmit is running / required label Nov 25, 2020
@mhevery
Copy link
Contributor

@mhevery mhevery commented Nov 25, 2020

presubmit

@mhevery mhevery removed action: presubmit A standard presubmit is running / required action: review labels Nov 25, 2020
Copy link
Contributor

@mhevery mhevery left a comment

reviewed-for: global-approvers

Copy link
Contributor

@mhevery mhevery left a comment

reviewed-for: global-approvers

@mhevery mhevery removed request for jelbourn and IgorMinar Nov 25, 2020
@mhevery mhevery self-assigned this Nov 25, 2020
@mhevery mhevery added the action: merge PR author is ready for this to merge label Nov 25, 2020
jessicajaniuk pushed a commit that referenced this issue Nov 25, 2020
…d" to "for" (#39646)

For element queries that return sufficiently large NodeList
objects, using spread syntax to populate the results array
causes a RangeError due to the call stack limit being reached.
This commit updates the code to use regular "for" loop instead.

Fixes #38551.

PR Close #39646
@angular-automatic-lock-bot
Copy link

@angular-automatic-lock-bot angular-automatic-lock-bot bot commented Dec 29, 2020

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Dec 29, 2020
@ngbot ngbot bot removed this from the needsTriage milestone Dec 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge PR author is ready for this to merge cla: yes P4 Low-priority issue that needs to be resolved refactoring Issue that involves refactoring or code-cleanup target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants