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

Migrate to firebase v9 modular sdk #243

Merged
merged 21 commits into from
Jul 12, 2022
Merged

Conversation

thatfiredev
Copy link
Member

@thatfiredev thatfiredev commented Jun 30, 2022

Description

This PR should update geofire to use the new modular firebase sdk (v9).
I simply copied the changes from #217 and made small improvements with as little code changes as possible to fix the build. These include:

  • replace the uglify plugin with terser
  • use the stable version of firebase v9 instead of the beta
  • bump the node version on GitHub actions to 10.x (minimum required by v9)

TODO:

  • Docs
  • Migration guide
  • Examples
  • Remove exp version
  • Major version bump

Code sample

@thatfiredev thatfiredev changed the title migrate to firebase v9 modular sdk [WIP] migrate to firebase v9 modular sdk Jun 30, 2022
@thatfiredev thatfiredev marked this pull request as draft June 30, 2022 21:10
@thatfiredev thatfiredev marked this pull request as ready for review July 1, 2022 18:42
@thatfiredev thatfiredev changed the title [WIP] migrate to firebase v9 modular sdk Migrate to firebase v9 modular sdk Jul 1, 2022
@coveralls
Copy link

coveralls commented Jul 1, 2022

Coverage Status

Coverage increased (+0.2%) to 92.377% when pulling d7392a3 on thatfiredev:ss-modular-sdk into d5250d5 on firebase:master.

@thatfiredev
Copy link
Member Author

@puf this is ready for review.

Please use "Squash and Merge" when you merge it.

@@ -42,15 +42,15 @@
"geofire-common": "5.2.0"
},
"peerDependencies": {
"firebase": "^2.4.0 || 3.x.x || 4.x.x || 5.x.x || 6.x.x || 7.x.x || 8.x.x"
"firebase": "9.x.x"
Copy link
Collaborator

@puf puf Jul 2, 2022

Choose a reason for hiding this comment

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

I'm still a bit sad about losing so much compatibility, but I guess having a /compat path might be a bridge too far here. Folks on older/compat versions of the JS SDK should stick to the older geofire-js release, since that hasn't changed significantly in years anyway. Should we mention that in the README?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added it to the migration guide here: thatfiredev@654fb00

Copy link
Collaborator

@puf puf left a comment

Choose a reason for hiding this comment

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

One small nit from me, and a potential improvement to the docs. With those, this LGTM

@thatfiredev thatfiredev requested a review from puf July 4, 2022 15:57
@puf puf merged commit 6cfd009 into firebase:master Jul 12, 2022
@thatfiredev thatfiredev mentioned this pull request Jul 13, 2022
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants