Fix #986: call requestAnimationFrame on window

[bmakuh]

Currently requestAnimationFrame/cancelAnimationFrame are getting called in another context besides window (or self). This throws an error as an illegal operation, so this just calls the function with the explicit context of whatever win is.

I'm happy to write tests for this, but I just don't really know how to test this sort of thing. If anyone has ideas, let me know and I'll add them to this PR.

[TrySound]

@bmakuh What context are you talking about? We have two browser and node. Node shouldn't call raf.

[bmakuh]

@TrySound in animationFrame.js, win can be one of window, self, or {}. If we only do raf.call(window, timeout), then that would break in Node.

[TrySound]

Still not clear. What the problem are you trying to achieve? raf is called only on scroll. Scroll does not exist in node.

[bmakuh]

As referenced in the title, it fixes Issue #986, where browsers throw hundreds of exceptions while you scroll because requestAnimationFrame has been copied off of window and so the value of this is now illegal. You get this error in Firefox:

TypeError: 'cancelAnimationFrame' called on an object that does not implement interface Window.

and this in Chrome:

Uncaught TypeError: Illegal invocation

If you want more context on the bug, there's more discussion on the issue: #986

[vlebedeff]

@TrySound

What context are you talking about? We have two browser and node. Node shouldn't call raf.

requestAnimationFrame and cancelAnimationFrame are assigned to raf and caf in a point-free style. Thus when you call raf or caf they're going to have a different this compared to the case when you explicitly call window.requestAnimationFrame(...) or window.cancelAnimationFrame(...). That said, IMHO raf and caf should either reference window.requestAnimationFrame.bind(window) / window.cancelAnimationFrame.bind(window) or explicitly told that window should be their this with call.

[TrySound]

Can you reproduce the problem in codesandbox?

[steppefox]

Instead of exporting win object and calling raf/caf with win, why you just don't want to export raf/caf with bound context win?

export const raf = request.bind(win);
export const caf = cancel.bind(win);

[bmakuh]

Good call @steppefox, I can change that. I do think that it happens in the commonjs build too though; we're using webpack 1 currently and support for ES modules wasn't added until webpack 2.

[steppefox]

@bmakuh Btw, please try to provide some example in demo repository if you have a time of course, because it really looks strange and guys won't approve it then. Or i'll provide an example with that error on weekends if you want, cuz I already have some demo app, need to change import of react-virtualized to ES here: https://github.com/steppefox/sandbox-react-contactlist

UPD. Already done it

[steppefox]

Btw, maybe I'm wrong but looks like the problem appears only in ES build, commonjs build works fine

[steppefox]

@TrySound https://github.com/steppefox/sandbox-react-contactlist here is an example repo with existing bug, please check it out) Just clone and npm install && npm start

[bmakuh]

@steppefox I just updated it to use .bind() instead, FYI. Also, thanks for the repro case. I was going to get to that, but it wouldn't have been for awhile. I appreciate it.

[bvaughn]

The repro project seemingly has unrelated errors for me when run locally:

Uncaught TypeError: classnames_1.default is not a function
at List.render (app.js:25947)

[steppefox]

@bvaughn yep, fusebox sometimes blowing up something... let me publish that repo to GH Pages, i think that's will be better

[Nauzer]

I'm having the same issue when using ES build. @bmakuh 's fix seems to work for me.

[jokklan]

This fix, solved the problem for me in #986! Thank you :)!

[bmakuh]

@jokklan if you need to use this before it gets merged in, I've published a fork with the fix in it: "@ben.makuh/react-virtualized": "9.18.6-beta.3"

[jokklan]

Thanks @bmakuh ! Awesome :)!

[uqmessias]

@bmakuh I'm facing a similar issue, but even your patch didn't solve my issue.

For some weird reason, the dist/es/utils/requestAnimationTimeout.js file is unable to be exported by Webpack on the project that I'm working on.

I renamed it to dist/es/utils/bla.js and it worked well.

Do you know what could cause this weird issue?

[bmakuh]

@uqmessias hmm, I'm not sure, but that maybe sounds like a separate issue?

[uqmessias]

@bmakuh yesterday, right after send you a message here, I submitted a PR (#1134 ) with some changes that worked for me.
Now I'm trying to extract some files from the project I'm working on in order to post it somewhere and show the issue happening on a real project.

Anyway, thanks for answering!

[domenkozar]

I can reproduce this on Chromium Version 94.0.4606.61 (Official Build) (64-bit)

This PR fixes the issue.

To reproduce follow https://github.com/domenkozar/parcel-web-components-css-modules-issue

[felipernb]

I'm observing the same issue when using react-virtualized via react-lazylog and building with parcel. This fix seems to solve it

[felipernb]

I know this might be seen as a bundling issue, as @steppefox mentioned years ago

Btw, maybe I'm wrong but looks like the problem appears only in ES build, commonjs build works fine

but what would be the downside of just merging this? It seems to me that it just makes the module more robust.

[yuxiang-projz]

reproduce when building with parcel

[develtype]

I had same issue when I build with parcel v2

[DaanRoet]

Looks like I'm facing the same issue with Next.js + TurboPack (beta) in development

[bvaughn]

Sorry for the very long delay in getting back to you about this!

I've just finished a large update for version 10 of react-virtualized (#1900): the library has been rewritten in TypeScript, every component is now a function component (React 18+ is required), and the source moved from source/ to lib/. Because of that, this PR can't be merged as-is. Version 10 still calls requestAnimationFrame/cancelAnimationFrame without binding them to window (in lib/utils/animationFrame.ts).

If you're still interested in this change, I'd welcome a new PR with it rebased onto the version-10 branch (or onto master once version 10 has been merged).