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

[v1] Replace setImmediate with requestAnimationFrame #1521

Merged
merged 1 commit into from Dec 28, 2020

Conversation

DrRefactor
Copy link
Contributor

@DrRefactor DrRefactor commented Dec 9, 2020

Description

requestAnimationFrame is called before frame is flushed, as soon as possible.
https://developer.mozilla.org/en-US/docs/Web/API/window/requestAnimationFrame

Fixes #1520

Changes

For example:

  • Changed setImmediate to requestAnimationFrame as node update scheduler for web

Screenshots / GIFs

Change can be seen clearly when using imperative Value.setValue(number) along with calling React.setState. React.setState is handled much faster in this scenario.

Background color in below example is updated via standard React state cycle, and position is set via ReanimatedNode.setValue. Both are fired in same synchronous scope (see below repo).

Before

setImmediate

After

requestAnimationFrame

Test code and steps to reproduce

https://github.com/DrRefactor/reanimated-issue-1520-repro

I've included instructions for running versions "before" and "after". I'm not sure this is the easiest way to do so though.

Checklist

  • Included code example that can be used to test this change
  • Ensured that CI passes

@nandorojo
Copy link
Contributor

This would be a great fix for web, thanks!

Copy link
Member

@jakub-gonet jakub-gonet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

There's a small note in the requestAnimationFrame docs that can be problematic for the users:

Be sure to always use the first argument (or some other method for getting the current time) to calculate how much the animation will progress in a frame, otherwise the animation will run faster on high refresh rate screens. Check the example below for a way to do this.

But using requestAnimationFrame instead of non-polyfilled setImmediate is better in every aspect so I think we can merge this one and fix this problem later.

@jakub-gonet jakub-gonet changed the title Use requestAnimationFrame to update AnimatedNode on web to make anima… Replace setImmediate with requestAnimationFrame Dec 28, 2020
@jakub-gonet jakub-gonet changed the title Replace setImmediate with requestAnimationFrame [v1] Replace setImmediate with requestAnimationFrame Dec 28, 2020
@jakub-gonet jakub-gonet merged commit 39905f2 into software-mansion:master Dec 28, 2020
@cltsang
Copy link
Contributor

cltsang commented Jan 18, 2021

To whom it may concern I have to use dynamic import to get it built on Next js
i.e.

// do this
import dynamic from 'next/dynamic';
const ScrollBottomSheet = dynamic(
  () => import('react-native-scroll-bottom-sheet'),
  { ssr: false }
);
// instead of this
import ScrollBottomSheet from 'react-native-scroll-bottom-sheet';

ref: https://stackoverflow.com/a/64261030/2437201

@nandorojo
Copy link
Contributor

nandorojo commented Jan 25, 2021

Hm, I didn't realize there was no polyfill to get requestAnimationFrame to work with SSR. @jakub-gonet is there any way we could add a check to see if the window exists when Platform.OS === 'web' to support SSR, and if not, use a no-op function? This is common practice for packages that support Server Side Rendering packages such as Next.js

@nandorojo
Copy link
Contributor

@cltsang
Copy link
Contributor

cltsang commented Jan 26, 2021

@nandorojo Yes it solves it. Thank you!
Didn't notice this react requirement before.

@nandorojo
Copy link
Contributor

Cool, where did you place the polyfill? pages/_app.tsx?

@cltsang
Copy link
Contributor

cltsang commented Jan 26, 2021

Yes I did.
import 'raf/polyfill';
at the first line.

@nandorojo
Copy link
Contributor

Got it. We should probably add this as a note to the web docs for Next.js/Gatsby/SSR support.

@nandorojo
Copy link
Contributor

Which version of reanimated is this shipped in? I'm using Expo SDK 40 + 2.0.0-rc.0, but it looks like it still uses set immediate. As a result, my Next.js app still sees this:

Screen Shot 2021-02-05 at 3 53 43 PM

@jakub-gonet
Copy link
Member

You can see the tag associated with the commit at the bottom of the description: this is included in rc.2. Bear in mind when you're using expo you can only upgrade libraries to version with non-native changes – Reanimated had those in rc.2 and rc.3.

jakub-gonet pushed a commit that referenced this pull request Mar 9, 2021
## Description

Fixes #1520 

requestAnimationFrame is called before frame is flushed, as soon as possible.
https://developer.mozilla.org/en-US/docs/Web/API/window/requestAnimationFrame

## Screenshots / GIFs

Change can be seen clearly when using imperative Value.setValue(number) along with calling React.setState. React.setState is handled much faster in this scenario.


Background color in below example is updated via standard React state cycle, and position is set via ReanimatedNode.setValue. Both are fired in same synchronous scope (see below repo).

### Before
![setImmediate](https://user-images.githubusercontent.com/21238529/101675012-7edf5d00-3a59-11eb-9884-1d9548d33a7c.gif)

### After
![requestAnimationFrame](https://user-images.githubusercontent.com/21238529/101675033-83a41100-3a59-11eb-966f-86628e641a50.gif)



## Test code and steps to reproduce
https://github.com/DrRefactor/reanimated-issue-1520-repro

I've included instructions for running versions "before" and "after". I'm not sure this is the easiest way to do so though.

## Checklist

- [x] Included code example that can be used to test this change
- [x] Ensured that CI passes


Co-authored-by: Paweł Więckowski <pawel.wieckowski@schange.com>
nandorojo added a commit to nandorojo/react-native-reanimated that referenced this pull request Nov 10, 2021
Use `requestAnimationFrame` on Web, instead of `setImmediate`.

See software-mansion#2621 and software-mansion#1521 for reference.
@nandorojo nandorojo mentioned this pull request Nov 10, 2021
6 tasks
piaskowyk pushed a commit that referenced this pull request Nov 23, 2021
Use `requestAnimationFrame` on Web, instead of `setImmediate`.

See #2621 and #1521 for reference.

## Description

Fix #2621
<!--
Description and motivation for this PR.

Inlude Fixes #<number> if this is fixing some issue.

Fixes # .
-->

## Changes

`setImmediate` breaks SSR on Web. `requestAnimationFrame` is better. This was already discussed and merged previously at #1521.

<!--
Please describe things you've changed here, make a **high level** overview, if change is simple you can omit this section.

For example:

- Added `foo` method which add bouncing animation
- Updated `about.md` docs
- Added caching in CI builds

-->

<!--

## Screenshots / GIFs

N/A.

### Before

Next.js wouldn't compile.

### After

It now does compile.

-->

## Test code and steps to reproduce

<!--
Please include code that can be used to test this change and short description how this example should work.
This snippet should be as minimal as possible and ready to be pasted into editor (don't exclude exports or remove "not important" parts of reproduction example)
-->

## Checklist

- [ ] Included code example that can be used to test this change
- [ ] Updated TS types
- [ ] Added TS types tests
- [ ] Added unit / integration tests
- [ ] Updated documentation
- [ ] Ensured that CI passes
github-merge-queue bot pushed a commit that referenced this pull request Sep 5, 2023
<!-- Thanks for submitting a pull request! We appreciate you spending
the time to work on these changes. Please follow the template so that
the reviewers can easily understand what the code changes affect. -->

## Summary
- expo/router#718
- [Twitter
thread](https://twitter.com/bhyoo99/status/1675302401677615104?s=20)

In Node.js environments such as when static rendering with Expo Router,
requestAnimationFrame is not defined. This results in an error when
using libraries that depend on react-native-reanimated like Drawer or
when using react-native-reanimated directly in Expo Router with static
rendering, causing a hydration mismatch due to rendering errors.

In this PR, I added a fallback to setImmediate if requestAnimationFrame
is not defined in `globalThis`. I did not simply replace
requestAnimationFrame with setImmediate because [`setImmediate` is
slower than
requestAnimationFrame](#1521)
in CSR environments. This issue is not applicable to static rendering
environments however, so setImmediate is fine there.

<!-- Explain the motivation for this PR. Include "Fixes #<number>" if
applicable. -->

## Test plan
- See [this
issue](expo/router#718 (comment)
Minimal reproducible example
<!-- Provide a minimal but complete code snippet that can be used to
test out this change along with instructions how to run it and a
description of the expected behavior. -->

---------

Co-authored-by: Tomasz Żelawski <tomasz.zelawski@swmansion.com>
Co-authored-by: Tomek Zawadzki <tomekzawadzki98@gmail.com>
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.

AnimatedNode should use requestAnimationFrame instead of setImmediate for web
5 participants