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 useFrameCallback fast refresh #4121

Merged
merged 2 commits into from
Mar 30, 2023
Merged

Conversation

mstach60161
Copy link
Contributor

@mstach60161 mstach60161 commented Feb 28, 2023

Summary

This PR fixes the problem of multiple callbacks after fast refresh

To prevent multiple loops, we use this if statement

// runCallback() should only be called after registering a callback,
// so if there is only one active callback, then it means that there were
// zero previously and the loop isn't running yet.
if (this.activeFrameCallbacks.size === 1) {
  requestAnimationFrame(loop);
}

The problem with fast refresh is that we unregister the callback and immediately register new.
Before unregistration is completed we register new one, so the previous loop doesn't stop.
We also start second loop, because we have one active frame. So in the end, after each fast refresh, we add new loop that execute the same callback.

To fix it, I added variable newCallId and pass it to each runCallbacks call, whenever we delete last item from active callbacks, we increment newCallId variable. Inside runCallbacks we check if the function execution is related with the newest callId. If not, we stop function execution.

Test plan

  • Example app
  • Flipper

Copy link
Member

@tomekzaw tomekzaw left a comment

Choose a reason for hiding this comment

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

Unfortunately, this PR doesn't work for me even on the most basic example. After fast refresh, no useFrameCallback callbacks are run.

import React from 'react';
import { View } from 'react-native';
import { useFrameCallback } from 'react-native-reanimated';

export default function AnimatedSensorExample() {
  useFrameCallback(() => {
    console.log(performance.now());
  });

  return <View />;
}

Copy link
Member

@tomekzaw tomekzaw left a comment

Choose a reason for hiding this comment

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

I can confirm this change successfully resolves the issue! ✅

Thanks @mstach60161 for this PR!

@mstach60161 mstach60161 added this pull request to the merge queue Mar 30, 2023
Merged via the queue into main with commit d62a6bf Mar 30, 2023
@mstach60161 mstach60161 deleted the @mstach60161/fix-useFrameCallback branch March 30, 2023 10:12
fluiddot pushed a commit to wordpress-mobile/react-native-reanimated that referenced this pull request Jun 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

This PR fixes the problem of multiple callbacks after fast refresh

To prevent multiple loops, we use this if statement 
```
// runCallback() should only be called after registering a callback,
// so if there is only one active callback, then it means that there were
// zero previously and the loop isn't running yet.
if (this.activeFrameCallbacks.size === 1) {
  requestAnimationFrame(loop);
}
```
The problem with fast refresh is that we unregister the callback and
immediately register new.
Before unregistration is completed we register new one, so the previous
loop doesn't stop.
We also start second loop, because we have one active frame. So in the
end, after each fast refresh, we add new loop that execute the same
callback.

To fix it, I added variable `newCallId` and pass it to each
`runCallbacks` call, whenever we delete last item from active callbacks,
we increment newCallId variable. Inside `runCallbacks` we check if the
function execution is related with the newest `callId`. If not, we stop
function execution.

## Test plan

- Example app
- Flipper
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.

None yet

3 participants