Initialize database before JS on iOS and create utility function that can be used in different callbacks whenever database access is needed - (didReceiveRemoteNotification in particular)
Details
Details
Build iOS app an real device, call this utility function from notification handling callback in every posssible app state: foreground, background and inactive. In each case examine logs to see that database is corectly initialized and accessible.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- marcin/eng-490
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
| native/ios/Comm/AppDelegate.mm | ||
|---|---|---|
| 65–66 ↗ | (On Diff #9946) | This might be ok, but just wanted to make sure. We're adding a property here that is the same as in CommSecureStoreIOSWrapper @interface CommSecureStoreIOSWrapper () @property(nonatomic, strong) EXSecureStore *secureStore; Do we really need to repeat that? |
| 252–254 ↗ | (On Diff #9946) | Do we need to call this every time attemptDatabaseInitialization is called? I guess it shouldn't matter as SQLiteQueryExecutor::initialize has call_once semantics. |
| native/ios/Comm/AppDelegate.mm | ||
|---|---|---|
| 65–66 ↗ | (On Diff #9946) | I received compile error without this attribute being added here and referencing it in attemptDatabaseInitialization. I will look for some documentation to make it clear. |
| 252–254 ↗ | (On Diff #9946) | I am not 100% sure what do you mean:
|
| native/ios/Comm/AppDelegate.mm | ||
|---|---|---|
| 65–66 ↗ | (On Diff #9946) | The reason this is necessary is that the property is not declared in the header file native/ios/Comm/CommSecureStoreIOSWrapper.h, so it's not visible. Maybe we should just declare it in the header file? |
| native/ios/Comm/AppDelegate.mm | ||
|---|---|---|
| 252–254 ↗ | (On Diff #9946) | I wan wondering if we can move this inside the if. But on the other hand, this should be a no-op and is safer, so we should probably keep it here. Theoretically it could happen in the future, that even when secureStore is not null, it was assigned by some other function and db was still uninitialized. |
| native/ios/Comm/AppDelegate.mm | ||
|---|---|---|
| 65–66 ↗ | (On Diff #9946) | Actually, now that I was assigned ENG-687 I am think about moving secure store initialization to CommSecureStoreIOSWrapper itself since we will probably want to call this function from UNNotificationServiceEXtenstion that should not call AppDelegate methods. |
| 252–254 ↗ | (On Diff #9946) | If I move secure store initialization to CommSecureStoreIOSWrapper method then It might indeed happen that secure store is initialized but SQLiteQueryExecutor not. That is why I implemented SQLiteQueryExecutor globals initialization in idempotent way. |
Comment Actions
Move secureStore property initialization to CommSecureStoreIOSWrapper implementation and remove redundant interface definition from AppDelegate.
| native/ios/Comm/CommSecureStoreIOSWrapper.h | ||
|---|---|---|
| 2 ↗ | (On Diff #10023) | I guess we should keep this newline |
| native/ios/Comm/CommSecureStoreIOSWrapper.mm | ||
| 30–44 ↗ | (On Diff #10023) | It looks like before calling initialize this object is unusable. Is it possible to initialize the object during sharedInstance creation? Or even in the constructor? |
Comment Actions
Move secure store initialization to CommSecureStoreIOSWrapper shared instance creation.
Comment Actions
It's great! We can try improving it even further, by creating a constructor that takes secureStore or even by moving the code that accesses moduleRegistry to the constructor itself, but it is not necessary.
| native/ios/Comm/AppDelegate.mm | ||
|---|---|---|
| 63 | I had to Google to learn the difference between -application:willFinishLaunchingWithOptions: and -application:didFinishLaunchingWithOptions:, and I suspect your other reviewers did too. In the future, when you're making a change like this, please save your reviewers some time and explain it inline. Better yet, separate it out into a different diff, and use the summary of that diff to explain the complicated part. | |
| 64 | I'm a little worried to see this called in -application:willFinishLaunchingWithOptions:, as the docs indicate:
It's not clear to me from the docs what is being referred to by "app's state", but it seems like it might be possible that the commSecureStore.get call in SQLiteQueryExecutor::initialize might rely on such "app state". @marcinwasowicz, can you research this in more detail and confirm that we are guaranteed that commSecureStore.get does not rely on any such "app state"? | |
| native/ios/Comm/CommSecureStoreIOSWrapper.h | ||
| 4 | If this is only used in the .mm file, shouldn't we move the #import statement to there? | |
| native/ios/Comm/AppDelegate.mm | ||
|---|---|---|
| 64 | commSecureStore.get relies entirely on EXSecureStore instance inside CommSecureStoreIOSWrapper. EXSecureStore is obtained from EXModuleRegistry that is obtained from EXModulesRegistryProvider which takes no arguments upon instantiation. One of the aims of this differential was to show that we can indeed instantiate EXSecureStore in any callback. From what I have read in apple documentation when willFinishLaunchingWithOptions is executed application is in inactive state, so there is no UI that user can interact with. The benefit of initializing SQLiteQueryExecutor in this callback is that we can use SQLite database to prepare some UI content for user. Please let me know if that is the information you were looking for. | |
| native/ios/Comm/CommSecureStoreIOSWrapper.h | ||
|---|---|---|
| 4 | This is valuable notice. I will introduce this change. | |
Comment Actions
commSecureStore.get relies entirely on EXSecureStore instance inside CommSecureStoreIOSWrapper. EXSecureStore is obtained from EXModuleRegistry that is obtained from EXModulesRegistryProvider which takes no arguments upon instantiation. One of the aims of this differential was to show that we can indeed instantiate EXSecureStore in any callback. From what I have read in apple documentation when willFinishLaunchingWithOptions is executed application is in inactive state, so there is no UI that user can interact with. The benefit of initializing SQLiteQueryExecutor in this callback is that we can use SQLite database to prepare some UI content for user. Please let me know if that is the information you were looking for.
I'm not sure this addresses my question... it's great that EXSecureStore takes no argument to initialize, but it still is accessing the app's internal state, right?
To answer my question, I think you'll need to dig deeper into -application:willFinishLaunchingWithOptions: and the docs I linked above. We need to figure out what is being referred to in the docs when they talk about the "app's state" that may not be available inside -application:willFinishLaunchingWithOptions:.
Comment Actions
Please hit "Plan Changes" after rebasing to make sure the diff doesn't go to your reviewers' queue!
Comment Actions
During my 1:1 meeting with @ashoat we found docs that support SQliteQueryExecutor initialization in willFinishLaunchingWithOptions callback. SQLiteQueryExecutor initialization require Expo Secure Store, which is a wrapper around keychain storage on iOS. We found a link to discussion on apple developer forum where developer mentions usage of keychain access in willFinishLaunchingWithOptions and Apple staff member does not point that it is unsafe or it may not work. Discussion can be viewed under the following link: https://developer.apple.com/forums/thread/114159.
Moreover we found a doc describing UI state restoration process: https://developer.apple.com/documentation/uikit/view_controllers/preserving_your_app_s_ui_across_launches/about_the_ui_restoration_process?language=objc. After reading this doc I came to conclusion that both willFinishLaunchingWithOptions and didFinishLaunchingWithOptions are callbacks responsible for UI state restoration. The difference between these two is that after didFinishLaunchingWithOptions applications view controllers are restored. Applications view controllers are not responsible for keychain access so if SQLiteQueryExecutor initialization worked well in didFinishLaunchingWithOptions then we can assume that doing it inside willFinishLaunchingWithOptions should also be safe.