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

[dev-launcher] move session fns from dev-menu to dev-launcher i #16124

Merged
merged 19 commits into from
Feb 9, 2022

Conversation

ajsmth
Copy link
Contributor

@ajsmth ajsmth commented Jan 29, 2022

Why

The new dev-launcher is now responsible for the user auth and session management - but these functions were still living in the dev-menu package - this PR aims to move the functionality into dev-launcher in an effort to clean things up

Android implementation is not yet done

How

Moved all the functions related to user auth from dev menu into the dev-launcher package - in order to do so I had to reimplement a few things in objective-c

Test Plan

  • unit tests should still pass
  • login / logout should work as expected in the 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).

@linear
Copy link

linear bot commented Jan 29, 2022

@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Jan 29, 2022
Copy link
Contributor

@esamelson esamelson left a comment

Choose a reason for hiding this comment

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

generally looks really good, just a few thoughts/questions

Comment on lines 3 to 4
export const apiEndpoint = __DEV__ ? `https://exp.host/--/graphql` : `https://exp.host/--/graphql`;
export const websiteOrigin = __DEV__ ? 'https://expo.dev' : 'https://expo.dev';
Copy link
Contributor

Choose a reason for hiding this comment

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

if this is an intentional change we should just get rid of the ternary operator entirely

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do! this was intentional as updating the api calls ended up being a bit more complicated - better suited for a different PR

@@ -44,3 +51,117 @@ export async function copyToClipboardAsync(content: string): Promise<null> {
export const clientUrlScheme = DevLauncher.clientUrlScheme;
export const installationID = DevLauncher.installationID;
export const isDevice = !!DevLauncher.isDevice;

Copy link
Contributor

Choose a reason for hiding this comment

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

do you think there is value in keeping the auth session / web browser stuff as a separate native module, even if it lives inside the dev launcher package?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that makes a lot of sense

Comment on lines -50 to -52
manager.sendEventToDelegateBridge(DevMenuExpoSessionDelegate.userLoginEvent, data: nil)
} else if wasLoggedIn && !isLoggedIn {
manager.sendEventToDelegateBridge(DevMenuExpoSessionDelegate.userLogoutEvent, data: nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

are these events unused now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes that's correct

@ajsmth ajsmth marked this pull request as ready for review February 2, 2022 19:54
ajsmth and others added 5 commits February 2, 2022 12:56
@ajsmth ajsmth requested a review from esamelson February 2, 2022 21:00
@ajsmth ajsmth changed the title [dev-launcher] move session fns from dev-menu to dev-launcher (WIP) [dev-launcher] move session fns from dev-menu to dev-launcher i Feb 2, 2022
@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Feb 2, 2022
packages/expo-dev-launcher/ios/EXDevLauncherAuth.h Outdated Show resolved Hide resolved
packages/expo-dev-launcher/ios/EXDevLauncherAuth.m Outdated Show resolved Hide resolved
packages/expo-dev-launcher/ios/EXDevLauncherAuth.m Outdated Show resolved Hide resolved
packages/expo-dev-launcher/ios/EXDevLauncherAuth.m Outdated Show resolved Hide resolved
packages/expo-dev-launcher/ios/EXDevLauncherAuth.m Outdated Show resolved Hide resolved
packages/expo-dev-launcher/ios/EXDevLauncherAuth.m Outdated Show resolved Hide resolved
packages/expo-dev-launcher/ios/EXDevLauncherAuth.m Outdated Show resolved Hide resolved
packages/expo-dev-launcher/ios/EXDevLauncherAuth.m Outdated Show resolved Hide resolved
ajsmth and others added 11 commits February 4, 2022 10:08
Co-authored-by: Łukasz Kosmaty <lukasz.kosmaty@swmansion.com>
Co-authored-by: Łukasz Kosmaty <lukasz.kosmaty@swmansion.com>
Co-authored-by: Łukasz Kosmaty <lukasz.kosmaty@swmansion.com>
Co-authored-by: Łukasz Kosmaty <lukasz.kosmaty@swmansion.com>
Co-authored-by: Łukasz Kosmaty <lukasz.kosmaty@swmansion.com>
Co-authored-by: Łukasz Kosmaty <lukasz.kosmaty@swmansion.com>
Co-authored-by: Łukasz Kosmaty <lukasz.kosmaty@swmansion.com>
Co-authored-by: Łukasz Kosmaty <lukasz.kosmaty@swmansion.com>
Co-authored-by: Łukasz Kosmaty <lukasz.kosmaty@swmansion.com>
Copy link
Contributor

@esamelson esamelson left a comment

Choose a reason for hiding this comment

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

🎉

@ajsmth ajsmth merged commit 6721364 into main Feb 9, 2022
@ajsmth ajsmth deleted the andrew/eng-3992-move-old-dev-menu-native-fns-to-dev branch February 9, 2022 00:14
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

4 participants