[Do Not Merge] => feat(expo): Migrate app to Expo #280
Conversation
damassi
requested review from
MounirDhahri,
araujobarret,
gkartalis and
brainbicycle
eslint-plugin-ft-flowAuthor: Unknown Description: Flowtype linting rules for ESLint by flow-typed Homepage: https://github.com/flow-typed/eslint-plugin-ft-flow#readme
react-nativeAuthor: Unknown Description: A framework for building native apps using React Homepage: https://reactnative.dev/
reactAuthor: Unknown Description: React is a JavaScript library for building user interfaces. Homepage: https://reactjs.org/
expo-dev-clientAuthor: 650 Industries, Inc. Description: Expo Development Client Homepage: https://docs.expo.dev/versions/latest/sdk/dev-client/
expoAuthor: Expo Description: The Expo SDK Homepage: https://github.com/expo/expo/tree/main/packages/expo
New dependencies added: |
damassi
added
Version: Major
There was a problem hiding this comment.
It really looks great!
Final steps involve things like CI and fastlane and updating commands for prod / beta builds (though i'm not sure we even need to do this; all this app is is a local storybooks for developing components. No reason we need to do anything at all but send out npm version via CI).
I think here it's used as a sanity check if something is breaking the build process of the app. We could experiment Expo's tooling instead of fastlane as part of the spike.
| "react-native-reanimated": "3.16.1", | ||
| "react-native-safe-area-context": "4.11.1", | ||
| "react-native-svg": "14.1.0", | ||
| "react-native-reanimated": "~3.10.1", |
There was a problem hiding this comment.
I think that's the necessary evil 🫠
| <key>NSPrivacyAccessedAPITypeReasons</key> | ||
| <array> | ||
| <string>CA92.1</string> | ||
| <string>E174.1</string> | ||
| <string>85F4.1</string> |
There was a problem hiding this comment.
this is maybe just because palette-mobiles privacy manifest wasn't accurate? but it might also be a side effect of pulling in expo which is a bit unfortunate, these show up on our privacy report card in App Store so adding extra keys is not ideal. Not the end of the world but not great.
There was a problem hiding this comment.
I bet we can tweak this -- these were added when expo ran the first pass auto-fix via
npx install-expo-modules@latest
I didn't take this PR all the way to completion (notably things like assets and the like configure via app.json), so reminder this was just a first pass to determine ease and to get a feel for the process.
If we want to proceed I'd want to take it one step further:
- free plan EAS builds
- delete ios and android directories
- Finish all of the static configuration so that we don't have to worry about asset configuration (splash, etc)
There was a problem hiding this comment.
eigen has more in here so really only matters if there is a diff between what is there now and what we get by bringing in expo but something to look out for
There was a problem hiding this comment.
btw these comments are me trying to get more of a read on what this would take in Eigen than a typical PR review so nothing in stone just my thoughts
There was a problem hiding this comment.
Oh totally understand. Just clarifying that what's seen here is technically incomplete 👍
| @@ -472,7 +515,7 @@ | |||
| GCC_WARN_UNINITIALIZED_AUTOS = YES_AGGRESSIVE; | |||
| GCC_WARN_UNUSED_FUNCTION = YES; | |||
| GCC_WARN_UNUSED_VARIABLE = YES; | |||
| IPHONEOS_DEPLOYMENT_TARGET = 12.4; | |||
| IPHONEOS_DEPLOYMENT_TARGET = 13.4; | |||
There was a problem hiding this comment.
i wonder if we have control over this? we are currently higher in eigen. not a deal breaker but something to look out for as these little keys have big consequences.
| </style> | ||
| <style name="Theme.App.SplashScreen" parent="AppTheme"> | ||
| <item name="android:windowBackground">@drawable/splashscreen</item> | ||
| </style> |
There was a problem hiding this comment.
does expo replace react-native-bootsplash in eigen's case?
| override fun getUseDeveloperSupport(): Boolean = BuildConfig.DEBUG | ||
|
|
||
| override val isNewArchEnabled: Boolean = BuildConfig.IS_NEW_ARCHITECTURE_ENABLED | ||
| override val isHermesEnabled: Boolean = BuildConfig.IS_HERMES_ENABLED |
There was a problem hiding this comment.
not important for palette but a call out if we do move to eigen we have customization in these native files for configuring some deps, I think Braze is one. We should try to understand what we have to do to either keep a custom version or generate a custom version or probably a lot of libraries have migration instructions.
| <uses-permission android:name="android.permission.READ_EXTERNAL_STORAGE"/> | ||
| <uses-permission android:name="android.permission.SYSTEM_ALERT_WINDOW"/> | ||
| <uses-permission android:name="android.permission.VIBRATE"/> | ||
| <uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE"/> |
There was a problem hiding this comment.
seems to have added a bunch of permissions, not a concern for palette-mobile, probably not a concern for eigen unless they differ from what is already there? but we should look out for it
| <intent> | ||
| <action android:name="android.intent.action.VIEW"/> | ||
| <category android:name="android.intent.category.BROWSABLE"/> | ||
| <data android:scheme="https"/> |
There was a problem hiding this comment.
have some custom domains in eigen for deeplinking, would be good to understand how that works in expo
| @@ -7,4 +7,8 @@ | |||
| # For more details, see | |||
| # http://developer.android.com/guide/developing/tools/proguard.html | |||
|
|
|||
| # react-native-reanimated | |||
| -keep class com.swmansion.reanimated.** { *; } | |||
| -keep class com.facebook.react.turbomodule.** { *; } | |||
There was a problem hiding this comment.
oh nice maybe this makes proguard adoption easier?
| "react-dom": "18.2.0", | ||
| "react-native": "0.75.4", | ||
| "react-native": "0.74.5", |
There was a problem hiding this comment.
this is one of my bigger concerns, locking us to specific dependencies. we fairly often have to upgrade things to workround issues and from what I can see here:
downgraded packages
- react native downgraded to 0.74.5 from 0.75.4
- react native reanimated to 3.10.1 from 3.16.1
- react from 18.2.0 to 18.3.1
- react-native-safe-area-context from 4.11.1 to 4.10.5
- @shopify/flash-list from 1.7.1 to 1.6.4
- react-native-pager-view from 6.5.0 to 6.3.0
upgraded packages:
- react-native-svg from 14.1.0 to 15.2.0
- typescript from 5.0.4 to 5.3.3
- @react-native-async-storage/async-storage 1.19.8 to 1.23.1
There was a problem hiding this comment.
if we have a major bug that we need to upgrade for it would be good to understand what our options are, does that mean we have to eject? or patching is our only option?
There was a problem hiding this comment.
maybe I will check also how often this is actually true we fairly often have to upgrade things to workround issues I feel like it is true but should be able to see in pr history
There was a problem hiding this comment.
fwiw, we can still keep the packages to the versions we want to and we don't have to be locked to what expo --fix suggests. The upgrades/downgrades are just suggestions we can ignore if we want to.
|
@MounirDhahri @damassi a thing that I noticed in the community for react native libraries that use expo: Seems like people are leaving expo outside the main library but they use it on an example app for running the library as an example there and the experience is pretty awesome and really fast + easy to contribute:
seems like we might need to treat palette-mobile a bit differently when it comes to an expo migration and treat it as a dependency and maybe have an example app that includes expo to be able to run it and develop on it taking the advantages of expo while at the same time not bloating the actual library code with extra deps. Happy to hear your thoughts on this. |
|
@gkartalis - yes, that's a good point. I don't have an idea yet how we can migrate palette-mobile, and still keep it working across two apps. I managed to get it only to work on Energy but as you mentioned we need to be careful with this migration to keep palette working for both repos. |
|
@gkartalis - I'm down with this manner of working on things! My take is that palette-mobile is somewhat trivial in the sense that its just a collection of components published to NPM, and a storybooks app to run. We'd want to (ideally) run But better would be to just have it all be on expo, delete ios and android folders, and keep it slim as possible. I'm not opinionated really; mostly think that Energy and Eigen would be the ones to benefit. |
|
@damassi note that also the libraries that I did mention do not include any android + ios folders since they are js only react native libraries but they do not include expo as a dep on their package.json. We could have something similar - like keeping the components on the |
| "styled-system": "^5.1.5" | ||
| "styled-system": "^5.1.5", | ||
| "react": "18.2.0", | ||
| "react-native": "0.74.5" |
There was a problem hiding this comment.
related to George's comment, I think one issue here is that moving these deps to hard versions in package.json presents a problem for consumers, ie. now Eigen needs to be on this specific RN version and same true for other libs. (I could be wrong not as familiar with nodes packaging) So we could either take George's approach of having an example app with the expo dependencies and keep the lib itself slim or find another way.
There was a problem hiding this comment.
I think in this instance react-native would be a peerDep and that would address the issue (assuming there's not other issues that would arise). Same with other libs that are shared from consumers of the app, keeping the dependencies array slender and scoped to (only) what palette-mobile might need when published and installed via npm. Would that work?
There was a problem hiding this comment.
yeah it sounds like it should!
Description
Similar to the Folio Expo migration this spikes on migrating palette mobile over as well.
TLDR: The toolchain is legit! I'm honestly kinda amazed by all of the things that are packed into
npx expo start, and impressed at how easy it was to get all of the deps fully configured withnpx expo-doctor.Given the abundant docs, I took a couple right turns initially, but then started over and everything worked very painlessly and was able to set up from scratch and run it in the iOS simulator in about 15 minutes.
Here are the steps I took:
npx install-expo-modules@latestnpx expo-doctor. Noticed there were a few package versions that weren't aligned with expo, a few minor versions awaynpx expo install --check. Installed everything, automatically updated package.jsonmainentry in package.json. This really stumped me initially before I bailed on trying to work around it. Expo reads this as the entrypoint for the app and it is not negotiable! There is no moreentryPointon app.json. Previously this pointed at thedist/index.js, the npm-publish friendly (compiled) version of our consumable@artsy/palette-mobilelib. Need to test whether lib still works in consumers without main. I think its fine.npx expo run:iosnpx expo run:androidandroidfolder, so delete it and let expo generate everything from scratch. Rerun.getMainComponentNameto match app name: "PaletteMobile"npx expo startFinal steps involve things like CI and fastlane and updating commands for prod / beta builds (though i'm not sure we even need to do this; all this app is is a local storybooks for developing components. No reason we need to do anything at all but send out npm version via CI).