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: avoid shared value reads during render #662

Merged
merged 1 commit into from
Oct 28, 2024

Conversation

kirillzyusko
Copy link
Owner

@kirillzyusko kirillzyusko commented Oct 27, 2024

📜 Description

Fixed popping up warnings when re-render happens if KeyboardAvoidingView is used.

💡 Motivation and Context

Reading .value during render is a bad approach because it violates rules of react, so we shouldn't use it anymore.

Fortunately we have only one place in the codebase - it's KeyboardAvoidingView. Technically I don't violate rules of

Initially I had 3 approaches in my head:

1️⃣ Precompute initial value via useMemo

The first approach I was thinking of was this:

const {height, progress} = useMemo(() => ({
  height: context.reanimated.height,
  progress: context.reanimated.progress,
}), []);

const heightWhenOpened = useSharedValue(-height);
const height = useSharedValue(-height);
const progress = useSharedValue(progress);
const isClosed = useSharedValue(progress === 0);

However later on in react docs I found this:

image

So I thought that in future potentially useMemo may be re-evaluated, so we'll get this warning again. Which is not desirable.

2️⃣ Memoize the initial value via useState

Another approach was the usage of useState:

  const [initialHeight] = useState(() => -reanimated.height.value);
  const [initialProgress] = useState(() => reanimated.progress.value);

  const heightWhenOpened = useSharedValue(initialHeight);
  const height = useSharedValue(initialHeight);
  const progress = useSharedValue(initialProgress);
  const isClosed = useSharedValue(initialProgress === 0);

In this case we derive value (kind of preparing them) and we are sure that those values will not be accidentally re-calculated (because in such way state will be initialized only one time).

The only downside is that we are creating additional variables and we use state not for its purpose (such values will never be updated actually).

3️⃣ create useLazySharedValue hook

The implementation could look like:

export function useLazySharedValue<Value>(initialValue: () => Value): SharedValue<Value> { // function instead of value
  const [mutable] = useState(() => makeMutable(initialValue()));
  useEffect(() => {
    return () => {
      cancelAnimation(mutable);
    };
  }, [mutable]);
  return mutable;
}

// and usage...

const progress = useLazySharedValue(() => context.reanimated.progress);

However I have next concerns in my head:

  • makeMutable is publicly exported, but I don't want to copy internal implementation of useSharedValue (even though this hook is a very simple). If it changes over time I'll have to replicate these changes in this package as well.

Keeping all possible ways in my head I decided to go with approach 2. In future I can adjust the logic if needed.

Closes #649

📢 Changelog

JS

  • prepare init values from shred value via useState(() => ;

🤔 How Has This Been Tested?

Tested locally on iPhone 16 Pro iOS 18.0.

📸 Screenshots (if appropriate):

Before After
Simulator.Screen.Recording.-.iPhone.16.Pro.-.2024-10-28.at.18.16.57.mp4
Simulator.Screen.Recording.-.iPhone.16.Pro.-.2024-10-28.at.18.17.42.mp4

📝 Checklist

  • CI successfully passed
  • I added new mocks and corresponding unit-tests if library API was changed

@kirillzyusko kirillzyusko added refactor You changed the code but it didn't affect functionality KeyboardAvoidingView 🧪 Anything related to KeyboardAvoidingView component labels Oct 27, 2024
@kirillzyusko kirillzyusko self-assigned this Oct 27, 2024
Copy link
Contributor

github-actions bot commented Oct 27, 2024

📊 Package size report

Current size Target Size Difference
158537 bytes 158244 bytes 293 bytes 📈

@kirillzyusko
Copy link
Owner Author

@IvanIhnatsiuk would you mind to have a look?

Comment on lines +11 to +12
const [initialHeight] = useState(() => -reanimated.height.value);
const [initialProgress] = useState(() => reanimated.progress.value);
Copy link
Contributor

Choose a reason for hiding this comment

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

hm, they already use useState under the hood https://github.com/software-mansion/react-native-reanimated/blob/main/packages/react-native-reanimated/src/hook/useSharedValue.ts#L19 and if we have a look at the react docs

initialState: The value you want the state to be initially. It can be a value of any type, but there is a special behavior for functions. This argument is ignored after the initial render.

So, are you sure that you need these changes?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah, but new REA 3.16 will complain if you use .value during render. So you should void the usage of .value during re-render (but you can use for the first render).

I had several approaches:

  1. To use useMemo to calculate initial values for reanimated.*.value - but this is not reliable, because useMemo in future may throw cache away and re-calculate memoization.

  2. To use useState to calculate initial value.

  3. to create own useLazySharedValue hook with next implementation:

export function useSharedValue<Value>(initialValue: () => Value): SharedValue<Value> { // function instead of value
  const [mutable] = useState(() => makeMutable(initialValue()));
  useEffect(() => {
    return () => {
      cancelAnimation(mutable);
    };
  }, [mutable]);
  return mutable;
}

If you have more ideas @IvanIhnatsiuk then I'll be happy to consider your thoughts 😊

Copy link
Contributor

Choose a reason for hiding this comment

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

hm, looks like it would be better to implement your own hook and remove it once people migrate to REA 3.16

Also, it helps to remove code duplication like this:

 const [initialHeight] = useState(() => -reanimated.height.value);
 const heightWhenOpened = useSharedValue(initialHeight);

and replace it with your own hook

 const heightWhenOpened = useLazySharedValue(() => -reanimated.height.value);

But anyway, it's up to you 🙂

Copy link
Owner Author

Choose a reason for hiding this comment

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

Well, there are few reasons why I don't want to implement my hook:

  • makeMutable is publicly exported, but I don't want to copy internal implementation of useSharedValue (even though this hook is a very simple). If it changes over time I'll have to replicate these changes in this package as well.
  • we have kind of code duplication, but thus we extract values and then reuse the value (I'm not sure if reading SharedValue on first render is an expensive operation or not, but I saw somewhere that it adds an overhead - but maybe mistaken).

This is why I think useState approach is better. anyway, it's pretty minor code modification - in future we may revisit this code 🙂

@kirillzyusko kirillzyusko force-pushed the fix/avoid-shared-value-reads-during-render branch from 484a50c to 42c30b6 Compare October 28, 2024 11:03
@kirillzyusko kirillzyusko marked this pull request as ready for review October 28, 2024 17:37
@kirillzyusko kirillzyusko merged commit e31a62a into main Oct 28, 2024
13 checks passed
@kirillzyusko kirillzyusko deleted the fix/avoid-shared-value-reads-during-render branch October 28, 2024 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
KeyboardAvoidingView 🧪 Anything related to KeyboardAvoidingView component refactor You changed the code but it didn't affect functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reanimated warning regarding value render during component render.
2 participants