Add required changes to run Hybrid App #32471

staszekscp · 2023-12-05T11:12:40Z

Details

The changes required to run Hybrid App. In this PR we tweak deeplink handling, and add patches required for running the app in OldDot. For more context, the patches are necessary to build app in a project with custom folder structure. They look for specific variables declared in Podfile or build.gradle, if the variables are not found, the patches will fallback to the current behaviour. Therefore they should introduce no change to NewDot.

Fixed Issues

$ #31511 #31510 #31509 #31508 #31512
PROPOSAL: Design Doc

Tests

Verify that no errors appear in the JS console
The app should not introduce any visible changes to NewDot

Offline tests

QA Steps

Verify that no errors appear in the JS console
The app should not introduce any visible changes to NewDot

PR Author Checklist

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

MacOS: Desktop

WoLewicki

I added some comments. Could you also explain why are all those patches needed?

WoLewicki · 2023-12-05T13:34:46Z

patches/@onfido+react-native-sdk+8.3.0.patch

         }
         classDirectories.setFrom(fileTree(
                 dir: 'build/intermediates/javac/debug/compileDebugJavaWithJavac/classes/com/onfido/reactnative/sdk',
+diff --git a/node_modules/@onfido/react-native-sdk/android/publish.gradle b/node_modules/@onfido/react-native-sdk/android/publish.gradle


We have a little bit different patch for this, maybe you should do it the same since our PR will also be merged soon most probably 😅 : https://github.com/Expensify/App/pull/31558/files#diff-38078bd96e4f2e196211b4e9eb902a3fd1a02b52238a7476813f92be71492105

I'm testing that out!

It works with your solution, so I'm going to use it instead 😄

WoLewicki · 2023-12-05T13:35:19Z

patches/@react-native+gradle-plugin+0.72.11.patch

+     project.pluginManager.withPlugin("com.android.library", action)
+   }
+
+  fun configureNamespaceForLibraries(appProject: Project) {


Why is it needed?

Would be nice to add some comment

Personally I have not changed this line - @mczernek Maybe you remember what was the reason behind this change?

Because, Gradle from some version requires each and every library to declare namespace, and since not all of them do, we need to do that for them.

Here's more info about that: react-native-community/discussions-and-proposals#671

WoLewicki · 2023-12-05T13:36:26Z

patches/@oguzhnatly+react-native-image-manipulator+1.0.5.patch

 android {
 -    compileSdkVersion 28
-+    compileSdkVersion 30
+    compileSdkVersion 33


Maybe we could bump it to 34 already?

I think we can leave it, since our project is currently configured to use 33

WoLewicki · 2023-12-05T13:39:07Z

src/libs/Navigation/AppNavigator/index.js

+        }
+
+        Navigation.isNavigationReady().then(() => {
+            Navigation.navigate(initUrl);


Is there a difference between null and undefined here? Or was there no particular reason that you used null?

What do you mean by that? I used null as initial value for the context that contains initUrl, but this part of code shouldn't be executed anyway if no url is passed in initialProps

Yeah, I'm asking if there is a reason why is null the initial value for the context, not undefined.

Actually I have just changed it to undefined, because after giving it more thought it is better then null (if no url is passed in initialProps, it's undefined anyway

WoLewicki · 2023-12-05T13:40:06Z

src/App.js

 import OnyxUpdateManager from './libs/actions/OnyxUpdateManager';
 import * as Session from './libs/actions/Session';
 import * as Environment from './libs/Environment/Environment';
+import InitialUrlContext from './libs/InitialUrlContext';


Is there a reason why those changes are here and not e.g. in Expensify file?

That's because we use initial props, that are passed directly to App.js file. 😄

WoLewicki · 2023-12-05T13:41:14Z

src/pages/LogOutPreviousUserPage.js

+        Linking.getInitialURL().then((url) => {
            const sessionEmail = props.session.email;
-            const isLoggingInAsNewUser = SessionUtils.isLoggingInAsNewUser(transitionURL, sessionEmail);
+            const transitionUrl = NativeModules.ReactNativeModule ? CONST.DEEPLINK_BASE_URL + initUrl : url;


Are you sure that ReactNativeModule is the best name for this new module? Maybe something more descriptive would be a better option?

That is true, it was named this way looking from OldDot perspective, where React Native is a new addition. I believe we could rename it eg. HybridAppModule

or HybridAppUtilsModule

kowczarz · 2023-12-05T14:27:27Z

patches/@react-native-community+cli-platform-ios+11.3.6.patch

+@@ -24,7 +24,6 @@ def use_native_modules!(config = nil)
+   # Resolving the path the RN CLI. The `@react-native-community/cli` module may not be there for certain package managers, so we fall back to resolving it through `react-native` package, that's always present in RN projects
+   cli_resolve_script = "try {console.log(require('@react-native-community/cli').bin);} catch (e) {console.log(require('react-native/cli').bin);}"
+   cli_bin = Pod::Executable.execute_command("node", ["-e", cli_resolve_script], true).strip
+-
+   if (!config)
+     json = []
+


Suggested change

@@ -24,7 +24,6 @@ def use_native_modules!(config = nil)

# Resolving the path the RN CLI. The `@react-native-community/cli` module may not be there for certain package managers, so we fall back to resolving it through `react-native` package, that's always present in RN projects

cli_resolve_script = "try {console.log(require('@react-native-community/cli').bin);} catch (e) {console.log(require('react-native/cli').bin);}"

cli_bin = Pod::Executable.execute_command("node", ["-e", cli_resolve_script], true).strip

-

if (!config)

json = []

WoLewicki · 2023-12-06T13:11:09Z

ios/NewExpensify/RCTBootSplash.m

  return dispatch_get_main_queue();
 }

+ (void)invalidateBootSplash {


Where do you call it?

While leaving NewDot - in OldDot we use reference to this file, and we call it in ReactNativeManager's close function. It is used to revert BootSplash to initial state, so it shows up again when NewDot gets opened multiple times.

staszekscp · 2024-02-05T14:50:50Z

Hey! I think everything is fine now 😄

situchan · 2024-02-05T14:54:45Z

What's the last change in InitialSettingsPage for?

staszekscp · 2024-02-05T15:00:01Z

I think after the Ideal Nav merge the file changed slightly, and for HybridApp we must adjust the item list - remove the Sign Out, and redirect to Expensify website. We add a button to come back to Expensify Classic instead

situchan · 2024-02-05T19:52:13Z

package.json

  "dependencies": {
    "@dotlottie/react-player": "^1.6.3",
-    "@expensify/react-native-live-markdown": "git+ssh://git@github.com/Expensify/react-native-live-markdown.git#c316611781f19815caebfed5540e0faf2a274785",
+    "@expensify/react-native-live-markdown": "git+ssh://git@github.com/Expensify/react-native-live-markdown.git#f0b63f41fa4c09bfa68439da0550b47bafd92fa2",


Please undo this change. Latest version will be bumped in #35852

situchan · 2024-02-06T09:52:54Z

@staszekscp please fix conflict. And Podfile.lock is still not commited

situchan · 2024-02-06T10:04:46Z

ios/Podfile.lock

-  - react-native-live-markdown (0.1.0):
-    - glog
-    - RCT-Folly (= 2022.05.16.00)
-    - React-Core


this was accidentally removed?

It's not removed, but renamed, you can see the package below. I suppose that in the version bump PR the pod install command was not executed, so Podfile.lock was not edited

I run pod install on latest main and diff doesn't occur

Ok, you're right, I know what happened. npm i didn't update react-native-live-markdown.podspec file without deleting node_modules

I'll push the change in a moment

situchan · 2024-02-06T10:05:45Z

ios/Podfile.lock

 PODFILE CHECKSUM: 0ccbb4f2406893c6e9f266dc1e7470dcd72885d2

-COCOAPODS: 1.13.0
+COCOAPODS: 1.14.3


Let's not bump pod version here. It's out of scope

Yeah, I was aware of that, it is fixed already

situchan · 2024-02-06T10:06:17Z

typecheck failing

situchan · 2024-02-06T11:05:32Z

ios/Podfile.lock

-  VisionCamera: 7d13aae043ffb38b224a0f725d1e23ca9c190fe7
-  Yoga: e64aa65de36c0832d04e8c7bd614396c77a80047
+  VisionCamera: fda554d8751e395effcc87749f8b7c198c1031be
+  Yoga: 13c8ef87792450193e117976337b8527b49e8c03


no need to update Yoga as well

Here I cannot do much about it, removing and generating Podfile.lock geve me the same result.

I could manually update the file, but I would rather not do it.

This checksum depends on device or OS where pod is being installed.
i.e. 13c8ef87792450193e117976337b8527b49e8c03 on your device and e64aa65de36c0832d04e8c7bd614396c77a80047 on my device

If you check commit history, this line has changed many times depending on PR author's device where pod is installed and committed

In that case what would you suggest?

nothing to suggest as this is RN upstream issue
either value is fine to me

I prefer e64aa65de36c0832d04e8c7bd614396c77a80047 since it seems to match with COCOAPODS: 1.13.0 on most devices

But anyway I think in that case we can leave it as it is, and the PR is currently ready 😄

situchan · 2024-02-06T12:46:04Z

@AndrewGable all yours

OSBotify · 2024-02-06T15:44:06Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

kavimuru · 2024-02-07T09:55:09Z

@situchan @AndrewGable could you explain what exactly the QA team to validate?

OSBotify · 2024-02-07T10:09:03Z

🚀 Deployed to staging by https://github.com/AndrewGable in version: 1.4.38-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	cancelled 🔪
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

situchan · 2024-02-07T10:10:53Z

@kavimuru nothing new to validate.
Just The app should not introduce any visible changes to NewDot which means same as "full regression test"

OSBotify · 2024-02-07T10:25:36Z

🚀 Deployed to staging by https://github.com/AndrewGable in version: 1.4.38-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2024-02-08T18:20:54Z

🚀 Deployed to production by https://github.com/marcaaron in version: 1.4.38-6 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

Krishna2323 · 2024-02-26T22:15:04Z

src/ROUTES.ts

+const HYBRID_APP_ROUTES = {
+    MONEY_REQUEST_CREATE: '/request/new/scan',
+} as const;
+


@staszekscp @situchan, coming from #35455 (comment) /request/new/scan is now replaced by a dynamic route now, can you pls let us know if we can change this to start/request/scan?

Hey! Thanks for tagging, and catching that! Actually it can stay as it is - the value is passed from OldDot, and then is mapped in NewDot to provide correct url. So unless the ROUTES.MONEY_REQUEST_CREATE.getRoute function changes, we don't have to update anything there. I'll keep it in mind for future changes, though!

@staszekscp thanks for the answer, there are typecheck fails so we need to update something to get rid of that, can you pls check this, just to be 100% sure.

staszekscp added 9 commits December 5, 2023 12:08

add required changes to run hybrid app

595a1b0

merge main

75ab2aa

update comment for appjs prop

cd14eca

move InitialUrlContext

a818a6b

add short explanation to InitialurlContext

0e228ca

add last lint to async storage patch

3ab29cc

remove line from App.js

7ce5254

run prettier

4da5437

cleanup

d33702c

WoLewicki reviewed Dec 5, 2023

View reviewed changes

staszekscp added 2 commits December 5, 2023 15:01

improve if statement

ae2bec1

rename ReactNativeModule to HybridAppModule

7796468

kowczarz approved these changes Dec 5, 2023

View reviewed changes

kowczarz reviewed Dec 5, 2023

View reviewed changes

update patch for onfido

405c29b

AndrewGable self-requested a review December 5, 2023 22:25

staszekscp added 3 commits December 6, 2023 10:08

add way to go back to Expensify Classic

4865330

tweak BootSplash to work with Hybrid App

c75ea56

switch to lodash filter

4dc32d4

WoLewicki reviewed Dec 6, 2023

View reviewed changes

staszekscp added 2 commits December 6, 2023 14:11

Merge branch 'main' into hybrid-app-changes

0d253f3

update InitialUrlContext

b53412c

staszekscp marked this pull request as ready for review December 6, 2023 14:26

staszekscp requested a review from a team as a code owner December 6, 2023 14:26

melvin-bot bot requested review from thesahindia and removed request for a team December 6, 2023 14:26

melvin-bot bot requested a review from AndrewGable February 2, 2024 17:48

staszekscp added 4 commits February 5, 2024 09:12

Merge branch 'main' into hybrid-app-changes

50777d7

update patch name

61c39c5

fix

71d628f

fix git statement

ae20949

update package.json wth fix to markdown

b0006f9

situchan reviewed Feb 5, 2024

View reviewed changes

staszekscp added 2 commits February 6, 2024 10:57

merge main

f67424c

commit podfile.lock

7f6464b

situchan reviewed Feb 6, 2024

View reviewed changes

staszekscp added 2 commits February 6, 2024 11:39

commit podfile.lock

46a7474

update podfile.lock

55ea149

situchan reviewed Feb 6, 2024

View reviewed changes

AndrewGable approved these changes Feb 6, 2024

View reviewed changes

AndrewGable merged commit ebc2b7d into Expensify:main Feb 6, 2024

github-actions bot mentioned this pull request Feb 7, 2024

Deploy Checklist: New Expensify 2024-02-07 #35983

Closed

49 tasks

s77rt mentioned this pull request Feb 8, 2024

[HOLD for payment 2024-02-20] [HOLD for payment 2024-02-19] [HOLD for payment 2024-02-15] [$500] Camera doesn't load when trying to smartscan a receipt #36098

Closed

6 tasks

Krishna2323 reviewed Feb 26, 2024

View reviewed changes

Add required changes to run Hybrid App #32471

Add required changes to run Hybrid App #32471

Uh oh!

Conversation

staszekscp commented Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

WoLewicki left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

staszekscp commented Feb 5, 2024

Uh oh!

situchan commented Feb 5, 2024

Uh oh!

staszekscp commented Feb 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

situchan commented Feb 6, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

staszekscp commented Dec 5, 2023 •

edited

Loading

staszekscp Feb 6, 2024 •

edited

Loading