feat: synchronous handler mount #538

kirillzyusko · 2024-08-06T16:51:58Z

📜 Description

Mount workletized handlers synchronously 😎

💡 Motivation and Context

Previously we were storing all worklet handlers in a global object and broadcasted events through all of them. However such approach had one big downside: updating shared value from JS is asynchronous (and if JS thread becomes busy, then mounting can take significant time).

In most of the cases it's not a big problem, but if keyboard moves when handler is not attached yet, then such keyboard movement is not getting tracked and as a result an actual keyboard position is not synchronized with shared values (if we are using components, such as KeyboardAvoidingView then they will not handle keyboard appearance properly).

I've considered two approaches how to fix it:

1️⃣ Distinguish which events were not sent to particular handler and send them after actual mount

That was the first idea and I thought it's quite perspective, but when I implemented it, I quickly realized, that:

it bring more hidden complexity;
it produces more race conditions - we can verify whether event was handled only in JS (only there we know which handlers are mounted and from UI thread we can send all ids of handlers that handled event), but we may have a situation, when handler skipped 10/30 events and handled last 20 events. In this case we shouldn't send these events, but we don't distinguish whether these events belong to the same group of events or not, so if we send them from JS to worklet, we may have a situation, where we handle last 20 events and only after that we handle first 10 events.

So at this point of time I realized, that it's not straightforward appraoch and started to look into different solutions.

2️⃣ Attach handler through JSI function

Another approach that I considered is attaching worklet handlers to the view directly (without intermediate handlers).
I discovered, that we call JSI function, which means that we have no delays or async stuff and was very inspired by that fact. I did some experiments and indeed it proved, that handlers can be attached synchronously.

However I discovered one use case - we still attach handler from useEffect (which is executed asynchronously) and worklet handlers are added via addUiBlock, so it's again asynchronous. So even with JSI we could have a delay up to 2 frames, which wasn't acceptable in certain cases.

At this point of time I thought that it's unreal to complete my objective, but then decided to try to use useSyncEffect. And with useSyncEffect it looks like we have only addUIBlock asynchronous and it's kind of acceptable (my handlers gets mounted faster, than keyboard events arrive).

So I re-worked code, added unit tests for useSyncEffect, run e2e and CI and everything seems to be pretty good so fat.

I know, that it's not very highly desirable to run synchronous events in react, but I feel like this approach is a last resort and want to try it. I also did performance benchmarks and didn't notice anything - UI gives 59/60 FPS and JS thread give 55 FPS (when navigating between screens). So I think this approach is an acceptable option that worth to try 🚀

📢 Changelog

Docs

mention that only react-native-reanimated@3.0.0 is a minimal supported version;

JS

added infrastructure for hook testing in src folder;
removed useSharedHandlers hook;
added useEventHandlerRegistration hook;
changed signature for setKeyboardHandler and setInputHandlers method;
assign ref to KeyboardControllerView;
add event-mapping and event-handler files;
added useSyncEffect hook;
removed useFocusedInputTextHandler, useFocusedInputSelectionHandler and uuid functions;

🤔 How Has This Been Tested?

Tested manually on iPhone 15 Pro.

Also verified that e2e tests are not failing.

📸 Screenshots (if appropriate):

Before	After
354689909-b7d1ff9f-958b-4c17-ba79-7b829bee2fd5.mp4	Screen.Recording.2024-08-08.at.18.02.53.mov

📝 Checklist

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

github-actions · 2024-08-06T17:08:55Z

📊 Package size report

Current size	Target Size	Difference
149304 bytes	147937 bytes	1367 bytes 📈

github-actions · 2024-08-07T11:59:15Z

PR Preview Action v1.4.7
Preview removed because the pull request was closed.
2024-08-08 16:12 UTC

argos-ci · 2024-08-07T12:00:25Z

The latest updates on your projects. Learn more about Argos notifications ↗︎

Build	Status	Details	Updated (UTC)
default (Inspect)	✅ No changes detected	-	Aug 8, 2024, 2:59 PM

…n` peer dependencies (#542) ## 📜 Description Minimal supported version of `react-native-reanimated` is `3.0.0`. ## 💡 Motivation and Context These changes were introduced in #538 In this PR I'm just changing version in `package.josn`. Follow up for #538 ## 📢 Changelog    ### JS - bump `react-native-reanimated` to `3.0.0`; ## 🤔 How Has This Been Tested? There is no way to test it 😀 ## 📝 Checklist - [x] CI successfully passed - [x] I added new mocks and corresponding unit-tests if library API was changed

@description

) ## 📜 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 #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 fix #575 is still relevant, as `ref` may still not be updated by the time when effect is called 🤷‍♂️ ## 📢 Changelog    ### 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

kirillzyusko added the 🚀 optimization You optimize something and it becomes working faster label Aug 6, 2024

kirillzyusko self-assigned this Aug 6, 2024

kirillzyusko added refactor You changed the code but it didn't affect functionality tests You added or changed unit tests labels Aug 6, 2024

kirillzyusko added 13 commits August 8, 2024 16:56

feat: synchronous handler mount

ca2549d

fix: TS

0c2e42a

fix: TS errors in internal.ts

24068cc

fix: finally fix TS

4214adf

chore: remove unused code

8150ba0

fix: autofixable eslint

6d81737

refactor: use own version of useSyncEffect with unit-tests

f5590d3

refactor: follow S in SOLID

5100141

refactor: don't use any type

a30a8fa

fix: removed console.log statements

2724e91

test: added more test for useSyncEffect

6fa7f1a

docs: update minimal supported REA version

f91275a

fix: compatibility with REA monorepo

a922738

kirillzyusko force-pushed the fix/sync-handlers-mount branch from 80a9d94 to a922738 Compare August 8, 2024 14:56

kirillzyusko marked this pull request as ready for review August 8, 2024 16:07

kirillzyusko merged commit cfc62b7 into main Aug 8, 2024
21 checks passed

kirillzyusko deleted the fix/sync-handlers-mount branch August 8, 2024 16:10

This was referenced Aug 9, 2024

fix: web compilation #541

Merged

chore: bump minimal react-native-reanimated version in package.json peer dependencies #542

Merged

kirillzyusko mentioned this pull request Aug 16, 2024

1.13.0 crashes on iOS #551

Closed

kirillzyusko mentioned this pull request Sep 4, 2024

fix: detached worklet handlers on Fabric when StrictMode enabled #579

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

feat: synchronous handler mount #538

feat: synchronous handler mount #538

Uh oh!

kirillzyusko commented Aug 6, 2024 •

edited

Loading

Uh oh!

github-actions bot commented Aug 6, 2024 •

edited

Loading

Uh oh!

github-actions bot commented Aug 7, 2024 •

edited

Loading

Uh oh!

argos-ci bot commented Aug 7, 2024 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

feat: synchronous handler mount #538

feat: synchronous handler mount #538

Uh oh!

Conversation

kirillzyusko commented Aug 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📜 Description

💡 Motivation and Context

1️⃣ Distinguish which events were not sent to particular handler and send them after actual mount

2️⃣ Attach handler through JSI function

📢 Changelog

Docs

JS

🤔 How Has This Been Tested?

📸 Screenshots (if appropriate):

📝 Checklist

Uh oh!

github-actions bot commented Aug 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📊 Package size report

Uh oh!

github-actions bot commented Aug 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

argos-ci bot commented Aug 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kirillzyusko commented Aug 6, 2024 •

edited

Loading

github-actions bot commented Aug 6, 2024 •

edited

Loading

github-actions bot commented Aug 7, 2024 •

edited

Loading

argos-ci bot commented Aug 7, 2024 •

edited

Loading