th3rdwave / react-native-safe-area-context

A flexible way to handle safe area insets in JS. Also works on Android and Web!
MIT License
2.08k stars 191 forks source link

Why memo onInsetChange then pass non-memo'ed style #426

Open Noitidart opened 10 months ago

Noitidart commented 10 months ago

I was thinking of making a PR to improve the memoization and adding imperative get API and also addListener API would that be accepted? The imperative and listener API is useful for things like reanimated.

For instance we wrap onInsetChange with useCallback but then pass style that referential changes every render. What is the purpose for memo'ing onInsetChange?

https://github.com/th3rdwave/react-native-safe-area-context/blob/930f703ca3337f407e2df13d1f4ec8e9ffcdb654/src/SafeAreaContext.tsx#L89

jacobp100 commented 10 months ago

You could do two PRs for this

Improving memoization would be useful. Also the use of useCallback there is a bit flaky - we should use the callback function for the setStates

I'm not too sure how the addListener would work. If you detail that a bit more I can tell you if there will be any issues

But yes - we do review all PRs 😊

Noitidart commented 10 months ago

Thanks very much. I'll do two PR proposals to start diacussion and will take it in the direction you recommend. I understand of proposals don't get accepted thanks for taking time to discuss and review.