Bind rAF and cAF to window (#16606) #16709
Conversation
When capturing local references of requestAnimationFrame and cancelAnimationFrame - bind them to the window object. Fixes facebook#16606
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
|
React: size: 🔺+1.1%, gzip: 🔺+0.3% Details of bundled changes.Comparing: 9444c87...0562fb5 react
scheduler
|
|
|
This issue occured when running in environments where window exists but the global |
|
I understand the reasoning. I’d just like someone to manually verify that this fixes the observable issue by doing a build and checking that. Could you please do it? |
|
Sorry I didn’t clarify. By “add a comment” I meant please add a comment in the source code. Just a couple of lines. |
Add a comment to clarify why we explicitly bind the functions to window.
Check specifically that requestAnimationFrame and cancelAnimationFrame are functions, and not just that they are truthy.
|
@gaearon I checked it. With a very minimal web-extension content script (repo here): const container = document.createElement('div');
container.id = "raf-test";
document.body.appendChild(container);
ReactDOM.render(React.createElement('div', null, `Hello rAF`), container);I Managed to reproduce it without the fix:
And with the fix it works:
|
There was a problem hiding this comment.
There could be other problems with using requestAnimationFrame in a background or extension script. The browser could choose to throttle requestAnimationFrame since the script is not associated with visible content, for instance. So I don't think binding to window is a reasonable solution. We should try to find a more comprehensive solution for when Scheduler runs in the background.
(However, we'll probably be switching away from requestAnimationFrame to MessageChannel, so this might not matter: #16408)
|
@acdlite do you think this change should be reverted? Or should it remain there to at least fix this specific issue (when trying to run React inside a web extension)? |
|
@liady Did you manage to fix this issue in your app/extension? Without using this PR, that is. |
|
@phaistonian no, still waiting for the final verdict on this PR... |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution. |
|
Closing this pull request after a prolonged period of inactivity. If this issue is still present in the latest release, please ask for this pull request to be reopened. Thank you! |
When capturing local references of requestAnimationFrame and cancelAnimationFrame - bind them to the window object.
Fixes #16606