Skip to content

[expo-firebase-*] Remove libraries #20979

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Feb 1, 2023
Merged

[expo-firebase-*] Remove libraries #20979

merged 5 commits into from
Feb 1, 2023

Conversation

keith-kurak
Copy link
Contributor

@keith-kurak keith-kurak commented Jan 27, 2023 •
edited
Loading

Why

We deprecated expo-firebase-analytics / expo-firebase-recaptcha (and hence, expo-firebase-core) in SDK 47, with removal planned in 48.

How

Remove expo-firebase-analytics, expo-firebase-recaptcha, expo-firebase-core packages. Updated dependencies in test apps/ Expo Go, removed Scoped Firebase class.

Needed to update com.google.firebase:firebase-core to 21.1.0 in expoview/build.gradle due to the below error. I think this was related to the Expo Firebase packages requiring this version anyway, which was likely inhibiting the error.

Duplicate class com.google.android.gms.internal.firebase_messaging.zza found in modules jetified-firebase-iid-20.0.2-runtime (com.google.firebase:firebase-iid:20.0.2) and jetified-firebase-messaging-22.0.0-runtime (com.google.firebase:firebase-messaging:22.0.0)
...
Duplicate class com.google.firebase.iid.FirebaseInstanceIdReceiver found in modules jetified-firebase-iid-20.0.2-runtime (com.google.firebase:firebase-iid:20.0.2) and jetified-firebase-messaging-22.0.0-runtime (com.google.firebase:firebase-messaging:22.0.0)

Test Plan

Tested building bare-expo and Expo Go on iOS and Android.

Checklist

NOTE: will open up a separate PR to update documentation; there's a decent amount of cross-linked stuff, don't want to miss that in a large-ish diff.

@linear
Copy link

linear bot commented Jan 27, 2023

ENG-7373 Remove expo-firebase-* libraries from Expo Go

@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Jan 27, 2023
@keith-kurak keith-kurak marked this pull request as ready for review January 27, 2023 20:26
@Simek
Copy link
Contributor

Simek commented Jan 28, 2023

We also need to remove docs for those packages, and setup the redirects to ensure that people don't land on 404.

tsapeta
tsapeta reviewed Jan 28, 2023
View reviewed changes
Copy link
Contributor

@tsapeta tsapeta left a comment

Choose a reason for hiding this comment

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

Usually we don't remove the entire package from the repo, even if we remove it from the SDK. We should keep package.json and README.md that states that this package is no longer maintained and also publish that to npm.

@keith-kurak
Copy link
Contributor Author

Usually we don't remove the entire package from the repo, even if we remove it from the SDK. We should keep package.json and README.md that states that this package is no longer maintained and also publish that to npm.

Good point- I was cribbing off of the Amplitude removal, and thought it was completely removed, but, per this comment, it turns out it was actually copied to a separate repo, with readme updated and set to read-only.

What's preferred in this case (copying to a new repo or leave package.json/ readme)? Copying perhaps makes it a little easier to see the final state before removal, I suppose.

@keith-kurak
Copy link
Contributor Author

We also need to remove docs for those packages, and setup the redirects to ensure that people don't land on 404.
Done - I added the redirects based on how they were done on this PR, but couldn't see a change (locally, it redirects back to SDK 47 docs), so appreciate an additional check there.

@tsapeta
Copy link
Contributor

tsapeta commented Jan 30, 2023

What's preferred in this case (copying to a new repo or leave package.json/ readme)? Copying perhaps makes it a little easier to see the final state before removal, I suppose.

If that sounds easier to you then yeah, go with copying 😉

@Simek
Copy link
Contributor

Simek commented Jan 30, 2023

Can you also remove entries for docs data generation from the file mentioned below?

@keith-kurak
Copy link
Contributor Author

What's preferred in this case (copying to a new repo or leave package.json/ readme)? Copying perhaps makes it a little easier to see the final state before removal, I suppose.

If that sounds easier to you then yeah, go with copying 😉

These are copied/ archived now:
https://github.com/expo/expo-firebase-analytics/
https://github.com/expo/expo-firebase-core/
https://github.com/expo/expo-firebase-recaptcha/

@keith-kurak keith-kurak requested a review from tsapeta February 1, 2023 19:52
@keith-kurak keith-kurak force-pushed the keith/eng-7373-remove-expo-firebase--libraries-from branch from 2eec0c2 to 784af64 Compare February 1, 2023 20:33
@expo-bot
Copy link
Collaborator

expo-bot commented Feb 1, 2023

Hi there! 👋 I'm a bot whose goal is to ensure your contributions meet our guidelines.

I've found some issues in your pull request that should be addressed (click on them for more details) 👇

⚠️ Suggestion: Missing changelog entries


Your changes should be noted in the changelog. Read Updating Changelogs guide and consider adding an appropriate entry to the following changelogs:

Generated by ExpoBot 🤖 against 784af64

Simek
Simek approved these changes Feb 1, 2023
View reviewed changes
Copy link
Contributor

@Simek Simek left a comment

Choose a reason for hiding this comment

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

Looks good from the docs side too! 👍

@keith-kurak keith-kurak merged commit d27f77b into main Feb 1, 2023
@keith-kurak keith-kurak deleted the keith/eng-7373-remove-expo-firebase--libraries-from branch February 1, 2023 20:57
thecodedrift added a commit to thecodedrift/expo that referenced this pull request Feb 2, 2023
@thecodedrift
Merge remote-tracking branch 'upstream/main' into expo-build-plugins/…
6302f0d 
…flipper

* upstream/main: (47 commits)
  [docs] Update Hermes guide to state that Hermes is the new default engine (expo#21047)
  chore: don't mark issues with the "Issue accepted" label as stale (expo#21058)
  Switch default JS engine to Hermes (expo#21001)
  [mail-composer][android] fix composeAsync not resolving after send/ discard (expo#20869)
  Update CHANGELOG.md (expo#21061)
  [core][iOS] Fix expo modules aren't added to global (expo#21037)
  [test-suite] Fix import in the Image example (expo#21043)
  [test-suite] fix video hanging (expo#21057)
  [av][ncl][go] fix audio and video qa issues (expo#21055)
  [tools] Selecting pull requests to label in the publish command (expo#20991)
  [document-picker] fill missing descriptions in `DocumentResult` type (expo#21040)
  [tools] Bump http-cache-semantics from 4.1.0 to 4.1.1 (expo#21049)
  Bump http-cache-semantics from 4.1.0 to 4.1.1 in /docs (expo#21050)
  [apps][yarn-workspace] replace deprecated activateKeepAwake
  update changelogs for react-native 0.71 upgrade (expo#20858)
  Upgrade react native to 0.71.2 (expo#21045)
  [go] update @shopify/react-native-skia to 0.1.172 (expo#21014)
  [stripe] Upgrade stripe to 0.23.1 (expo#20964)
  [expo-firebase-*] Remove libraries (expo#20979)
  [docs] Update expo-secure-store to add info about Export compliance (expo#21021)
  ...
@Simek Simek mentioned this pull request Feb 13, 2023
Merged
@Simek Simek mentioned this pull request Feb 13, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ide ide Awaiting requested review from ide

@brentvatne brentvatne Awaiting requested review from brentvatne

@amandeepmittal amandeepmittal Awaiting requested review from amandeepmittal

@kbrandwijk kbrandwijk Awaiting requested review from kbrandwijk

2 more reviewers

@Simek Simek Simek approved these changes

@tsapeta tsapeta tsapeta approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
bot: suggestions ExpoBot has some suggestions
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@keith-kurak @Simek @tsapeta @expo-bot