You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
fix: detached worklet handlers on Fabric when StrictMode enabled (kirillzyusko#579)
## 📜 Description
On fabric with `StrictMode` enabled worklet handlers were not working.
It was because of the fact that effects are fired twice, and current
implementation of `useSyncEffect` simply added handler and unmounted it
when effect is fired twice intentionally.
## 💡 Motivation and Context
Original PR that introduced these changes was
kirillzyusko#538
Theoretically this problem could be fixed by using a version like:
```tsx
import { useEffect, useRef } from "react";
import type { DependencyList } from "react";
/**
* @description
* Equivalent to `useEffect` but will run the effect synchronously, i. e. before render.
*
* @param {effect} - imperative function
* @param {deps} - if present, effect will only activate if the values in the list change
*
* @author Kiryl Ziusko
* @SInCE 1.13.0
* @Version 1.0.0
*/
const useSyncEffect: typeof useEffect = (effect, deps) => {
const cachedDeps = useRef<DependencyList | undefined | null>(null);
const areDepsEqual = deps?.every(
(el, index) => cachedDeps.current && el === cachedDeps.current[index],
);
const cleanupRef = useRef<(() => void) | void>();
if (!areDepsEqual || !cachedDeps.current) {
cleanupRef.current?.();
cleanupRef.current = effect();
cachedDeps.current = deps;
}
useEffect(() => {
// strict mode double effect invocation handling
if (deps !== cachedDeps.current) {
cleanupRef.current = effect();
cachedDeps.current = deps;
}
}, deps);
useEffect(() => {
return () => {
cleanupRef.current?.();
cachedDeps.current = null;
};
}, []);
};
export default useSyncEffect;
```
But in this case:
- effects are not running synchronously (second run from react in
`StrictMode` will run it asynchronously anyway because it'll be
triggered from `useEffect`);
- we mutate ref during render which is also not desirable in concurrent
react;
- the `StrictMode` fix can not be covered by unit tests, which indicated
that we go too deeply and probably doing something wrong if it can not
be covered by unit tests.
So keeping that in my head I decided to make a step back and rewrite
this part - now instead of `useSyncEffect` I will use `useLayoutEffect`.
In this case I'll still run effects slightly earlier than plain
`useEffect` and it should be alright for now. I've tested Expensify app
with these changes and now I don't see advantages of using
`useSyncEffect` over `useLayoutEffect` - both of them give roughly
speaking the same result.
I also thought about using `useInsertionEffect` (I'm not css-in-js
library developer, but still) - but in this case I also didn't discover
any advantages and react warns that it's better to stick to default
`useEffect`/`useLayoutEffect` hooks. Potentially we can use
`useInsertionEffect` but it should be a last resort and for now I prefer
to use `useSyncEffect` 😅
> [!IMPORTANT]
> Even though we migrated to default lifecycles it still worth to note,
that fixkirillzyusko#575
is still relevant, as `ref` may still not be updated by the time when
effect is called 🤷♂️
## 📢 Changelog
<!-- High level overview of important changes -->
<!-- For example: fixed status bar manipulation; added new types
declarations; -->
<!-- If your changes don't affect one of platform/language below - then
remove this platform/language -->
### JS
- remove `useSynceEffect` hook;
- use `useLayoutEffect` instead of `useEffect` hook.
## 🤔 How Has This Been Tested?
Tested on iPhone 15 Pro, iOS 17.5 (FabricExample app in `StrictMode` and
Expensify app).
## 📝 Checklist
- [x] CI successfully passed
- [x] I added new mocks and corresponding unit-tests if library API was
changed
0 commit comments