expo / eas-cli

Fastest way to build, submit, and update iOS and Android apps
https://docs.expo.dev/eas/
MIT License
676 stars 74 forks source link

[ENG-12122][eas-cli] don't prompt users to set push notifications by default if they don't have the `expo-notifications` installed #2343

Closed szdziedzic closed 1 month ago

szdziedzic commented 1 month ago

Why

https://linear.app/expo/issue/ENG-12122/dont-prompt-people-to-set-push-notifications-by-default-if-they-dont

It was noticed that these prompts not always make sense and may be annoying

How

don't prompt users to set push notifications by default if they don't have the expo-notifications installed

Test Plan

test manually

with promptToConfigurePushNotifications not set and expo-notifications not installed:

Screenshot 2024-04-24 at 18 26 25

with promptToConfigurePushNotifications not set and expo-notifications installed:

Screenshot 2024-04-24 at 18 27 38
linear[bot] commented 1 month ago

ENG-12122 Don't prompt people to set push notifications by default if they don't have `expo-notifications` installed

szdziedzic commented 1 month ago

/changelog-entry chore Don't prompt users to set push notifications by default if they don't have the expo-notifications installed

github-actions[bot] commented 1 month ago

Size Change: -392 B (0%)

Total Size: 51.4 MB

Filename Size Change
./packages/eas-cli/dist/eas-linux-x64.tar.gz 51.4 MB -392 B (0%)

compressed-size-action

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 25.00000% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 53.67%. Comparing base (bc2f6e6) to head (ed4558a).

:exclamation: Current head ed4558a differs from pull request most recent head 90c5172. Consider uploading reports for the commit 90c5172 to get more accurate results

Files Patch % Lines
...-cli/src/credentials/ios/IosCredentialsProvider.ts 20.00% 3 Missing and 1 partial :warning:
packages/eas-cli/src/project/projectUtils.ts 33.34% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #2343 +/- ## ========================================== - Coverage 53.68% 53.67% -0.00% ========================================== Files 525 525 Lines 19266 19282 +16 Branches 4066 4072 +6 ========================================== + Hits 10341 10348 +7 - Misses 8200 8208 +8 - Partials 725 726 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

brentvatne commented 1 month ago

very reasonable! i requested review from @douglowder and @christopherwalter since they work on notifications

szdziedzic commented 1 month ago

awesome, thanks!

brentvatne commented 1 month ago

btw, we should check the onesignal docs to ensure that this doesn't cause any issues with their instructions

github-actions[bot] commented 1 month ago

✅ Thank you for adding the changelog entry!