Skip to content
This repository has been archived by the owner on Jan 18, 2024. It is now read-only.

[expo-cli] add option to assign created push key to current project #3098

Merged
merged 2 commits into from
Jan 21, 2021

Conversation

cruzach
Copy link
Contributor

@cruzach cruzach commented Jan 20, 2021

fixes expo/expo#11656

After generating a new push key, we should give the option to assign that key to the current project context, if there is one. Otherwise, it can be confusing that you need to create the key, then assign it

@@ -38,6 +38,11 @@ export class CreateIosPush implements IView {
log('Successfully created Push Notification Key\n');
displayIosUserCredentials(pushKey);
log();

Copy link
Contributor

@wkozyra95 wkozyra95 Jan 20, 2021

Choose a reason for hiding this comment

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

This action is called from other places e.g. as part of the setup before build where this behaviour would not be correct.

This was intended to simple CRUD action that results in the creation of a new push key, if the intention is that ensuring valid push key is assigned to the app then logic for that should be different and is already present in build command.

If it's unclear that this command won't assign dist cert to the app then maybe it's better to add some warnings. If you want to modify this action without affecting build command then create new action that is using CreateIosPush class.

async open(ctx: Context): Promise<> {
  await runCredentialsManager(ctx, new CreateIosPush(accountName));
  // some code you want to add
}

and replace it here https://github.com/expo/expo-cli/blob/master/packages/expo-cli/src/credentials/views/Select.ts#L93

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah I see, thanks for catching that

Changed it and created a new class that extends CreateIosPush instead - dcae9ca

Copy link
Member

@quinlanj quinlanj left a comment

Choose a reason for hiding this comment

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

hm, this does seem like the better user experience. Thanks for this PR @cruzach. I'll work on changing this in eas too

@cruzach cruzach merged commit 9485c82 into master Jan 21, 2021
@cruzach cruzach deleted the @cruzach/ios-push-key-fix branch January 21, 2021 15:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[docs] iOS Push Notifications Setup Unclear for Bare Workflow
3 participants