[No QA] Replace deprecated functions: runOnUI, runOnJS, runOnRuntime, executeOnUIRuntimeSync#74486
Conversation
runOnUI and runOnJSrunOnUI, runOnJS, runOnRuntime
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
runOnUI, runOnJS, runOnRuntimerunOnUI, runOnJS, runOnRuntime, executeOnUIRuntimeSync
| @@ -0,0 +1,7 @@ | |||
| // runOnUISync crashes on web not because browsers lack a UI thread concept, | |||
There was a problem hiding this comment.
This suggests that runOnUISync (as execcuteOnUIRuntimeSync) is not supported on the web
There was a problem hiding this comment.
The code comment starts by saying what doesn’t happen and only afterward explains the real reason. It would be clearer to state directly that runOnUISync fails because web doesn’t support it, instead of leading with a negation.
For example:
// runOnUISync isn't supported in web by react-native-worklets, so we use the async alternative on web.
|
|
blazejkustra
left a comment
There was a problem hiding this comment.
Very elegant, minor comments @GCyganek 👏
| ### [react-native-worklets+0.6.0.patch](react-native-worklets+0.6.0.patch) | ||
|
|
||
| - Reason: [File with mocks](https://github.com/software-mansion/react-native-reanimated/blob/main/packages/react-native-worklets/src/mock.ts) hasn't been released yet for `react-native-worklets`, but it was needed to fix test failures after migration to `react-native-reainmated` v4. | ||
| - Upstream PR/issue: N/A |
There was a problem hiding this comment.
There is no issue in worklets repo? Let's create it 👍
| @@ -0,0 +1,7 @@ | |||
| // runOnUISync crashes on web not because browsers lack a UI thread concept, | |||
There was a problem hiding this comment.
The code comment starts by saying what doesn’t happen and only afterward explains the real reason. It would be clearer to state directly that runOnUISync fails because web doesn’t support it, instead of leading with a negation.
For example:
// runOnUISync isn't supported in web by react-native-worklets, so we use the async alternative on web.
| import {runOnRuntime} from 'react-native-reanimated'; | ||
| import {scheduleOnRuntime} from 'react-native-worklets'; | ||
|
|
||
| function runOnLiveMarkdownRuntime<Args extends unknown[], ReturnType>(worklet: (...args: Args) => ReturnType) { |
There was a problem hiding this comment.
I think we should convert runOnLiveMarkdownRuntime to scheduleOnLiveMarkdownRuntime for the consistency with scheduleOnRN from react-native-worklets.
There was a problem hiding this comment.
Thanks, good point
src/components/withKeyboardState.tsx
Outdated
| 'worklet'; | ||
|
|
||
| runOnJS(setIsKeyboardAnimating)(false); | ||
| scheduleOnRN(setIsKeyboardAnimating, true); |
There was a problem hiding this comment.
Previously this line was runOnJS(setIsKeyboardAnimating)(false); so it should be false instead of true.
| scheduleOnRN(setIsKeyboardAnimating, true); | |
| scheduleOnRN(setIsKeyboardAnimating, false); |
| beforeEach(() => { | ||
| jest.useFakeTimers(); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| jest.useRealTimers(); | ||
| }); |
There was a problem hiding this comment.
scheduleOnUI mock uses setTimeout (contrary to the mock that we had for react-native-animated counterpart), so without using
act(() => {
jest.advanceTimersByTime(1);
});the test will fail
| scheduleOnLiveMarkdownRuntime(() => { | ||
| 'worklet'; | ||
|
|
||
| mentionsSharedVal.set(availableLoginsList); |
There was a problem hiding this comment.
Ideally we'd like scheduleOnLiveMarkdownRuntime to have the same signature as scheduleOnUI does, i.e. without the need to call the returned function, like this:
-scheduleOnLiveMarkdownRuntime(fn)(...args); // currently you need to add `();` at the end
+scheduleOnLiveMarkdownRuntime(fn, ...args);|
Ready |
|
@tomekzaw could you please take a look again so I can mark it as ready to review? 👉 👈 |
|
No product considerations, removing my review |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/deetergp in version: 9.2.60-0 🚀
|
|
🚀 Deployed to staging by https://github.com/deetergp in version: 9.2.60-0 🚀
|
|
🚀 Deployed to staging by https://github.com/deetergp in version: 9.2.60-0 🚀
|
|
🚀 Deployed to production by https://github.com/grgia in version: 9.2.60-2 🚀
|
Explanation of Change
after bumping Reanimated to v4
runOnUI,runOnJS,runOnRuntimeandexecuteOnUIRuntimeSyncfromreact-native-reanimatedgot deprecated and should be replaced withscheduleOnUI,scheduleOnRN,scheduleOnRuntimeandrunOnUISyncfromreact-native-workletssource: https://docs.swmansion.com/react-native-reanimated/docs/guides/migration-from-3.x/
Fixed Issues
Follow-up to #69469
$ N/A
PROPOSAL: N/A
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop