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

[ios][application] Migrate to Expo Modules API #24871

Merged
merged 8 commits into from Oct 17, 2023

Conversation

reichhartd
Copy link
Contributor

Why

Migrate to expo modules API.

How

Followed typical migration steps.

There is a breaking change: getIosPushNotificationServiceEnvironmentAsync() now returns a string of a union type instead of an enum integer. I changed this because I thought it made more sense from a user perspective, it is recommended in the Expo Modules API documentation that way, and TypeScript enums should be avoided in general.

Test Plan

Tested on my physical iPhone 11 Pro.

Before After
old new

Checklist

@reichhartd reichhartd requested a review from ide as a code owner October 15, 2023 12:44
@expo-bot expo-bot added the bot: needs changes ExpoBot found things that don't meet our guidelines label Oct 15, 2023
@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: needs changes ExpoBot found things that don't meet our guidelines labels Oct 16, 2023
Comment on lines -121 to -127
export enum ApplicationReleaseType {
UNKNOWN = 0,
SIMULATOR = 1,
ENTERPRISE = 2,
DEVELOPMENT = 3,
AD_HOC = 4,
APP_STORE = 5,
Copy link
Member

Choose a reason for hiding this comment

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

Prefer the enum over type unions IMO (will defer to @tsapeta). If the issue is that the runtime values of the enums are human-unfriendly, we could make a breaking change to make them be strings (enum { UNKNOWN = 'UNKNOWN', ... }).

Copy link
Collaborator

@alanjhughes alanjhughes left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! I've left some feedback. For now, I think we won't make the breaking change

@expo-bot expo-bot added bot: suggestions ExpoBot has some suggestions and removed bot: passed checks ExpoBot has nothing to complain about labels Oct 17, 2023
@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Oct 17, 2023
Copy link
Collaborator

@alanjhughes alanjhughes left a comment

Choose a reason for hiding this comment

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

Last few small things and should be good to go

packages/expo-application/ios/ApplicationExceptions.swift Outdated Show resolved Hide resolved
packages/expo-application/ios/ApplicationExceptions.swift Outdated Show resolved Hide resolved
packages/expo-application/ios/ApplicationExceptions.swift Outdated Show resolved Hide resolved
reichhartd and others added 3 commits October 17, 2023 13:47
Co-authored-by: Alan Hughes <30924086+alanjhughes@users.noreply.github.com>
Co-authored-by: Alan Hughes <30924086+alanjhughes@users.noreply.github.com>
Co-authored-by: Alan Hughes <30924086+alanjhughes@users.noreply.github.com>
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

5 participants