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

I linked the correct issue in the ### Fixed Issues section above
I wrote clear testing steps that cover the changes made in this PR
- I added steps for local testing in the Tests section
- I added steps for the expected offline behavior in the Offline steps section
- I added steps for Staging and/or Production testing in the QA steps section
- I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
- I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
I included screenshots or videos for tests on all platforms
I ran the tests on all platforms & verified they passed on:
- Android: Native
- Android: mWeb Chrome
- iOS: Native
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- MacOS: Desktop
I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
I followed proper code patterns (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
  - If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I followed the guidelines as stated in the Review Guidelines
I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
I verified that if a function's arguments changed that all usages have also been updated correctly
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If the PR modifies the form input styles:
- I verified that all the inputs inside a form are aligned with each other.
- I added Design label so the design team can review the changes.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

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

@@ -15,3 +15,49 @@ index b4c7106..d5083d3 100644
         }
         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

@@ -7,13 +7,13 @@ index 3a1a548..fe030bb 100644

 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

@@ -25,11 +26,21 @@ import useDefaultDragAndDrop from './hooks/useDefaultDragAndDrop';
 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

            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

@@ -26,6 +26,12 @@ - (dispatch_queue_t)methodQueue {
  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

@@ -59,7 +59,7 @@
  },
  "dependencies": {
    "@dotlottie/react-player": "^1.6.3",
-    "@expensify/react-native-live-markdown": "git+ssh://[email protected]/Expensify/react-native-live-markdown.git#c316611781f19815caebfed5540e0faf2a274785",
+    "@expensify/react-native-live-markdown": "git+ssh://[email protected]/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

  Yoga: e64aa65de36c0832d04e8c7bd614396c77a80047

 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
25 checks passed

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

Conversation

staszekscp commented Dec 5, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

WoLewicki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staszekscp commented Feb 5, 2024

situchan commented Feb 5, 2024

staszekscp commented Feb 5, 2024

Choose a reason for hiding this comment

situchan commented Feb 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

situchan commented Feb 6, 2024

Choose a reason for hiding this comment

staszekscp Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

situchan commented Feb 6, 2024

OSBotify commented Feb 6, 2024

kavimuru commented Feb 7, 2024

OSBotify commented Feb 7, 2024

situchan commented Feb 7, 2024

OSBotify commented Feb 7, 2024

OSBotify commented Feb 8, 2024

Choose a reason for hiding this comment

staszekscp Feb 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staszekscp commented Dec 5, 2023 •

edited

Loading

staszekscp Feb 6, 2024 •

edited

Loading

staszekscp Feb 28, 2024 •

edited

Loading