New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
@mczernek/app loader migration #8438
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great change! 👍
packages/@unimodules/react-native-adapter/android/src/main/AndroidManifest.xml
Show resolved
Hide resolved
🛠 Suggested fixes:📋 Missing changelogApply suggested changes:diff --git a/packages/unimodules-app-loader/CHANGELOG.md b/packages/unimodules-app-loader/CHANGELOG.md
index e23a2d85f6..e0e228f655 100644
--- a/packages/unimodules-app-loader/CHANGELOG.md
+++ b/packages/unimodules-app-loader/CHANGELOG.md
@@ -8,4 +8,5 @@
### 🐛 Bug fixes
+- @mczernek/app loader migration. ([#8438](https://github.com/expo/expo/pull/8438) by [@mczernek](https://github.com/mczernek))
- Fixed `appLoaderRegisteredForName` to not only check if a loader class name is in the cache for the provided name but also verifies that the cached and current class name match. When migrating from managed to bare, the class name cache needs to be updated. ([#8292](https://github.com/expo/expo/pull/8292) by [@thorbenprimke](https://github.com/thorbenprimke)) |
…nsume it in AppLoaderProvider.
…ting in the meantime.
4156f32
to
d45b44f
Compare
🛠 Suggested fixes:📋 Missing changelogApply suggested changes:diff --git a/packages/unimodules-app-loader/CHANGELOG.md b/packages/unimodules-app-loader/CHANGELOG.md
index 0b0d87d5..754689d2 100644
--- a/packages/unimodules-app-loader/CHANGELOG.md
+++ b/packages/unimodules-app-loader/CHANGELOG.md
@@ -8,6 +8,8 @@
### 🐛 Bug fixes
+- @mczernek/app loader migration ([#8438](https://github.com/expo/expo/pull/8438) by [@mczernek](https://github.com/mczernek))
+
## 1.1.0 — 2020-05-27
### 🐛 Bug fixes or merge this pull request: #8517 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sooo many hacks there 😞 But looks like there is no better way right now 😞
Left a few comments, but besides of them
- All those new files could use Kotlin 😄
- Would be great if you could add some comments around those repository classes & methods and especially in places where we do these hacks
packages/expo-task-manager/android/src/main/java/expo/modules/taskManager/TaskService.java
Outdated
Show resolved
Hide resolved
.../android/src/main/java/expo/modules/taskManager/repository/BareTasksAndEventsRepository.java
Outdated
Show resolved
Hide resolved
.../android/src/main/java/expo/modules/taskManager/repository/BareTasksAndEventsRepository.java
Show resolved
Hide resolved
...droid/src/main/java/expo/modules/taskManager/repository/ManagedTasksAndEventsRepository.java
Outdated
Show resolved
Hide resolved
...ager/android/src/main/java/expo/modules/taskManager/repository/TasksAndEventsRepository.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still one comment, but generally looks good now 👍
Could you also update PR's title?
Co-authored-by: Tomasz Sapeta <1714764+tsapeta@users.noreply.github.com>
Why
If an application is being ejected, and some user updates its previously standalone (managed) application to ejected one, crash is observed when attempting to use AppLoader. Additionally, simple fix to prevent the crash causes helps, but tasks are not being transferred between versions correctly.
How
Root cause of the problem is the fact, that name of explicit AppLoader class implementation is persisted in SharedPreferences. At the same time, it differs between managed and bare workflow.
Current implementation defines names of loader in meta-data. This should cause it to be changed accordingly when migrating from managed to bare workflow.