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 request animation frame context #3016

Merged

Conversation

johman10
Copy link
Contributor

@johman10 johman10 commented Jun 12, 2019

This fixes: #2933
Issue has been in place since 3.4.3 and later, 3.4.2 is working as expected.
Reproduction of the issue can be found here:
Edit icy-grass-vv0y6
Check the console of the browser to see the error mentioned.

Basically the requestAnimationFrame is extracted from the window, which gives it the wrong context when triggered during animation time. The context when triggering a requestAnimationFrame should always be the window. So binding it when reassigning it fixes this issue.

I haven't added tests to cover for this change. I'm not sure how to approach that, anyone that can help me and figure that out?

@Conduitry
Copy link
Member

Summary of findings, in case for some reason you don't want to read the billion comments on this PR:

This seems to happen when the bundled app is using the CommonJS versions of modules. There's code that attempts to call exports.raf(...) which will be the wrong context. The tidiest way to fix this doing seem to be to just use export let raf = cb => requestAnimationFrame(cb); as this takes care of the context issues and the treeshaking issues, and doesn't break tests.

Having tests for this doesn't seem at all practical, because it would involve using the original requestAnimationFrame (currently we override it during tests so we can control time better), and it would also involve bundling using the CommonJS versions of internals (which we are not set up for at all).

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

Successfully merging this pull request may close these issues.

TypeError: Illegal invocation on transition
4 participants