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

Consider adding cancelAnimationFrame polyfill #4779

Closed
gaearon opened this issue Oct 28, 2017 · 11 comments · Fixed by #4780
Closed

Consider adding cancelAnimationFrame polyfill #4779

gaearon opened this issue Oct 28, 2017 · 11 comments · Fixed by #4780

Comments

@gaearon
Copy link
Contributor

gaearon commented Oct 28, 2017

Related to #4545 and #4568.

It's not necessary for React, but I think most apps expect them to be available together. Since if you don't have access to the original rAF polyfill source you can't implement the cAF polyfill.

@SimenB
Copy link
Member

SimenB commented Oct 28, 2017

Technically you can pass it to clearTimeout, but that's not a very intuitive API.

It will hopefully land in jsdom itself soon, but just doing global.cancelAnimationFrame = global.clearTimeout should be good enough in the meantime.

@gaearon
Copy link
Contributor Author

gaearon commented Oct 28, 2017

By the way, by reading jsdom discussion, I'm not quite sure which behavior they'll pick.

Would it make sense to change the polyfilll to coalesce calls into 1/60 timer? e.g. if you call it twice synchronously it should probably schedule only once.

@gaearon
Copy link
Contributor Author

gaearon commented Oct 28, 2017

Now that I think about it, I realize I don't actually remember how rAF works 😛

@SimenB
Copy link
Member

SimenB commented Oct 28, 2017

When you schedule 2 at the same time, they should be fired at the same time. That's actually not the way it works in my PR to JSDOM, there they'll be scheduled 1/60 after calling all the time... Probably needs a queue of some sort.

E.g. run this in chrome, and see the now parameter be the same.

requestAnimationFrame((t)=>{console.log('hello', t)}); requestAnimationFrame((t)=>{console.log('hello', t)});

EDIT: Wait, I'm confusing myself. Need to double check the spec (or just use raf as suggested below)

@gaearon
Copy link
Contributor Author

gaearon commented Oct 28, 2017

How do you feel about just using raf package as a fallback? Their logic looks good to me.

@SimenB
Copy link
Member

SimenB commented Oct 28, 2017

I'm fine with just using raf. I was gonna use it initially (sent chrisdickinson/raf#35 for the API I wanted), but ended up just implementing it myself as we wanted to copy what react recommended as polyfill

@SimenB
Copy link
Member

SimenB commented Oct 28, 2017

Only issue with raf is that they will schedule using the host's (i.e. the context that Jest itself executes in) setTimeout instead of the one from jsdom. Probably not an issue in practice, though

@gaearon
Copy link
Contributor Author

gaearon commented Oct 28, 2017

we wanted to copy what react recommended as polyfill

Heh. We only recommended that because that's what we used in our tests, and it's the simplest thing that works. I don't think we really meant to endorse it though 😛

@SimenB
Copy link
Member

SimenB commented Oct 29, 2017

Both requestAnimationFrame and cancelAnimationFrame has now landed in JSDOM. Not released yet, but IMO that means there's no reason to mess with adding raf to Jest

@gaearon
Copy link
Contributor Author

gaearon commented Oct 29, 2017

Agree (if you can easily update).

@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants