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

animationsSettled() hanging forever #226

Open
Turbo87 opened this issue Jun 9, 2020 · 6 comments
Open

animationsSettled() hanging forever #226

Turbo87 opened this issue Jun 9, 2020 · 6 comments

Comments

@Turbo87
Copy link
Contributor

Turbo87 commented Jun 9, 2020

Describe the bug

We lately started seeing an issue in our test suite on CI where tests were sometimes running into QUnit timeouts (60s) and it appears to be related to animationsSettled(). It seems that calling await animationsSettled() never resolves for some reason.

What appears to be related is that in some other test we are using @sinon/fake-timers to fake the system time, which is essentially overriding window.Date. My suspicion is that this interacts with the internal clock of ember-animated in some way that leaks between tests.

To Reproduce

Unfortunately I have not been able to reproduce this issue yet locally 😞

Expected behavior

I expect animationsSettled() to resolve once the animations are done, instead of waiting forever and causing the test to time out.

Desktop (please complete the following information):

The test suite is running in a Docker container with Google Chrome.

@spruce
Copy link

spruce commented Aug 31, 2020

I was having a similar problem. For me the finalizeAnimation function seemed not to stop.

I wanted to have nice drag and drop mechanics for my kanban style application.
So I used the drag motion which was shown in the living animations presentatoin by Edward Falkner and modified it a little to fit my needs.
I wanted to update the server when the animations stopped, but they never did. (even though no element was moving)

I created a twiddle which showcases the problem: https://ember-twiddle.com/ca133bd34dd3cfabbd866b20667b1a32

@nickschot
Copy link
Contributor

nickschot commented Jun 28, 2021

Debugged this a little as well and in the specific case I'm debugging it seems that occasionally the first yield rAF call used internally by the animationsSettled test-helper never resolves (or the entire task is cancelled maybe?).

rAF call: https://github.com/ember-animation/ember-animated/blob/master/addon/services/motion.ts#L210
rAF implementation: https://github.com/ember-animation/ember-animated/blob/master/addon/-private/concurrency-helpers.ts#L40

Debugging further to figure out why...

@nickschot
Copy link
Contributor

nickschot commented Jun 29, 2021

So it's definitely the Sinon fake-timers interfering with ember-animated's clock which is used by i.e. the Tween's and some of the concurrency-helpers.

https://github.com/ember-animation/ember-animated/blob/master/addon/-private/concurrency-helpers.ts#L129

Making an explicit ref to Date inside the concurrency-helpers file instead of relying on the global might "fix" this? Not sure if there's a better workaround. I don't see a need for ember-animated's clock to not be the browser's Date though, so @ef4 if you're fine with this that might be the most straightforward solution and I can make a PR.

edit: does not seem to be enough.

@nickschot
Copy link
Contributor

nickschot commented Jun 29, 2021

The cause in this specific case seemed to be Sinon fake-timers stubbing requestAnimationFrame, which can be disabled through their configuration: https://github.com/sinonjs/fake-timers#var-clock--faketimersinstallconfig . This seems to resolve the issue we had.

@spruce
Copy link

spruce commented Mar 7, 2024

I found out how to fix my problem at least.

stop() {
this.stopped = true;
if (this.state && isPromise(this.state.value)) {

In this snippet I added the following line after this.stopped = true;:

    this.resolve({stopped:true});

For me this always resolves the promise which allows me to do work after the animation is done.

This looks like a solution to these problems to me. I'd like to contribute this back. Let me know.

@spruce
Copy link

spruce commented Mar 7, 2024

Apparently I did so long ago already. #492 @SergeAstapov Do you care to merge the PR?

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

No branches or pull requests

3 participants