Skip to content
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

[expo-dev-launcher] take 2 at SDK 44 plugin compatibility #15570

Merged
merged 5 commits into from
Dec 15, 2021

Conversation

esamelson
Copy link
Contributor

Why

when testing #15562 on the sdk-44 branch I found there were still a few issues -- (1) some expo-modules stuff would be inadvertently removed from AppDelegate, even though the project would build, and (2) tests were failing.

How

(commit 1) added the correct logic for SDK 44's initializeReactNativeApp function (these regexes are getting so messy, we can't move to expo module initialization soon enough πŸ˜… ), and added tests with a corresponding SDK 44 AppDelegate.m.
(commit 2) fixed wrong string constant that was causing failures for SDK 43 and below
(commit 3) fixed tests on the Android side that were logging warnings. @lukmccall is it correct to say that this config plugin cannot handle projects where createReactActivityDelegate isn't already overridden in MainActivity? I modified the tests as such, but if that isn't the case, this warning needs to be changed.

Test Plan

All tests pass. Manually tested in a blank SDK 44 app; cherry picked these commits onto sdk-44 and then used patch-package to patch entire the expo-dev-launcher package to match sdk-44 HEAD. Ran expo prebuild, saw no errors, eyeballed the code changes, and tested the following scenarios:

βœ… project builds on iOS and Android
βœ… iOS: can open app served by expo start --dev-client
βœ… iOS: can open app served by expo start --dev-client --force-manifest-type=expo-updates
βœ… Android: can open app served by expo start --dev-client
βœ… Android: can open app served by expo start --dev-client --force-manifest-type=expo-updates
βœ… expo install expo-updates@0.11.2, expo prebuild --clean, project builds
βœ… iOS (with expo-updates integration): can open app served by expo start --dev-client
βœ… iOS (with expo-updates integration): can open app served by expo start --dev-client --force-manifest-type=expo-updates
βœ… iOS (with expo-updates integration): can open published app
βœ… Android (with expo-updates integration): can open app served by expo start --dev-client
βœ… Android (with expo-updates integration): can open app served by expo start --dev-client --force-manifest-type=expo-updates
βœ… Android (with expo-updates integration): can open published app

Checklist

  • Documentation is up to date to reflect these changes (eg: https://docs.expo.dev and README.md).
  • This diff will work correctly for expo build (eg: updated @expo/xdl).
  • This diff will work correctly for expo prebuild & EAS Build (eg: updated a module plugin).

@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Dec 15, 2021
Copy link
Contributor

@lukmccall lukmccall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM βœ…

@lukmccall lukmccall merged commit e94611e into master Dec 15, 2021
@lukmccall lukmccall deleted the @eric/dc-plugin-fix-44 branch December 15, 2021 12:26
@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Dec 15, 2021
lukmccall pushed a commit that referenced this pull request Dec 15, 2021
Co-authored-by: Łukasz Kosmaty <lukasz.kosmaty@swmansion.com># Why

when testing #15562 on the sdk-44 branch I found there were still a few issues -- (1) some expo-modules stuff would be inadvertently removed from AppDelegate, even though the project would build, and (2) tests were failing.

# How

(commit 1) added the correct logic for SDK 44's `initializeReactNativeApp` function (these regexes are getting so messy, we can't move to expo module initialization soon enough πŸ˜… ), and added tests with a corresponding SDK 44 AppDelegate.m.
(commit 2) fixed wrong string constant that was causing failures for SDK 43 and below
(commit 3) fixed tests on the Android side that were logging warnings. @lukmccall is it correct to say that this config plugin cannot handle projects where `createReactActivityDelegate` isn't already overridden in MainActivity? I modified the tests as such, but if that isn't the case, [this warning](https://github.com/expo/expo/blob/4ca5ff72ef3abcc30cb7936eaf9c4b97fed9a5ad/packages/expo-dev-launcher/plugin/src/withDevLauncher.ts#L205-L209) needs to be changed.

# Test Plan

All tests pass. Manually tested in a blank SDK 44 app; cherry picked these commits onto sdk-44 and then used patch-package to patch entire the expo-dev-launcher package to match sdk-44 HEAD. Ran expo prebuild, saw no errors, eyeballed the code changes, and tested the following scenarios:

βœ… project builds on iOS and Android
βœ… iOS: can open app served by `expo start --dev-client`
βœ… iOS: can open app served by `expo start --dev-client --force-manifest-type=expo-updates`
βœ… Android: can open app served by `expo start --dev-client`
βœ… Android: can open app served by `expo start --dev-client --force-manifest-type=expo-updates`
βœ… expo install expo-updates@0.11.2, expo prebuild --clean, project builds
βœ… iOS (with expo-updates integration): can open app served by `expo start --dev-client`
βœ… iOS (with expo-updates integration): can open app served by `expo start --dev-client --force-manifest-type=expo-updates`
βœ… iOS (with expo-updates integration): can open published app
βœ… Android (with expo-updates integration): can open app served by `expo start --dev-client`
βœ… Android (with expo-updates integration): can open app served by `expo start --dev-client --force-manifest-type=expo-updates`
βœ… Android (with expo-updates integration): can open published app
DominickVale pushed a commit to DominickVale/expo that referenced this pull request Dec 15, 2021
Co-authored-by: Łukasz Kosmaty <lukasz.kosmaty@swmansion.com># Why

when testing expo#15562 on the sdk-44 branch I found there were still a few issues -- (1) some expo-modules stuff would be inadvertently removed from AppDelegate, even though the project would build, and (2) tests were failing.

# How

(commit 1) added the correct logic for SDK 44's `initializeReactNativeApp` function (these regexes are getting so messy, we can't move to expo module initialization soon enough πŸ˜… ), and added tests with a corresponding SDK 44 AppDelegate.m.
(commit 2) fixed wrong string constant that was causing failures for SDK 43 and below
(commit 3) fixed tests on the Android side that were logging warnings. @lukmccall is it correct to say that this config plugin cannot handle projects where `createReactActivityDelegate` isn't already overridden in MainActivity? I modified the tests as such, but if that isn't the case, [this warning](https://github.com/expo/expo/blob/4ca5ff72ef3abcc30cb7936eaf9c4b97fed9a5ad/packages/expo-dev-launcher/plugin/src/withDevLauncher.ts#L205-L209) needs to be changed.

# Test Plan

All tests pass. Manually tested in a blank SDK 44 app; cherry picked these commits onto sdk-44 and then used patch-package to patch entire the expo-dev-launcher package to match sdk-44 HEAD. Ran expo prebuild, saw no errors, eyeballed the code changes, and tested the following scenarios:

βœ… project builds on iOS and Android
βœ… iOS: can open app served by `expo start --dev-client`
βœ… iOS: can open app served by `expo start --dev-client --force-manifest-type=expo-updates`
βœ… Android: can open app served by `expo start --dev-client`
βœ… Android: can open app served by `expo start --dev-client --force-manifest-type=expo-updates`
βœ… expo install expo-updates@0.11.2, expo prebuild --clean, project builds
βœ… iOS (with expo-updates integration): can open app served by `expo start --dev-client`
βœ… iOS (with expo-updates integration): can open app served by `expo start --dev-client --force-manifest-type=expo-updates`
βœ… iOS (with expo-updates integration): can open published app
βœ… Android (with expo-updates integration): can open app served by `expo start --dev-client`
βœ… Android (with expo-updates integration): can open app served by `expo start --dev-client --force-manifest-type=expo-updates`
βœ… Android (with expo-updates integration): can open published app
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: passed checks ExpoBot has nothing to complain about
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants