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

feat(storage): Firebase JS SDK v9 modular API #6958

Merged
merged 15 commits into from
Mar 23, 2023
Merged

Conversation

russellwheatley
Copy link
Member

Description

See title.

Related issues

Release Summary

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
    • Yes
  • My change supports the following platforms;
    • Android
    • iOS
  • My change includes tests;
    • e2e tests added or updated in packages/\*\*/e2e
    • jest tests added or updated in packages/\*\*/__tests__
  • I have updated TypeScript types that are affected by my change.
  • This is a breaking change;
    • Yes
    • No

Test Plan


Think react-native-firebase is great? Please consider supporting the project with any of the below:

@vercel
Copy link

vercel bot commented Mar 2, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
react-native-firebase ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 21, 2023 at 2:31PM (UTC)
1 Ignored Deployment
Name Status Preview Comments Updated
react-native-firebase-next ⬜️ Ignored (Inspect) Mar 21, 2023 at 2:31PM (UTC)

@codecov
Copy link

codecov bot commented Mar 17, 2023

Codecov Report

Merging #6958 (d79e2ba) into main (e118597) will decrease coverage by 0.24%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##               main    #6958      +/-   ##
============================================
- Coverage     53.90%   53.66%   -0.24%     
  Complexity      718      718              
============================================
  Files           228      229       +1     
  Lines         11303    11355      +52     
  Branches       1812     1815       +3     
============================================
  Hits           6092     6092              
- Misses         4877     4926      +49     
- Partials        334      337       +3     

@russellwheatley russellwheatley marked this pull request as ready for review March 17, 2023 10:58
Lyokone
Lyokone previously approved these changes Mar 17, 2023
Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

This looks really good - I had two really minor thoughts for the changes implemented, trivial really but need a look

Main structural thought is that we've had one of these modular PRs go in with a delete of the non-modular tests, since the modular e2e includes the compatibility testing, that actually seems like a viable path forward, and reduces e2e time. We have also had a few go in like this where the e2e non-modular tests are effectively duplicated, with no delete of the original tests.

What do you prefer here? I lean towards a delete of the non-modular tests personally but it is a weakly held opinion.

@@ -50,7 +50,7 @@ import database from '@react-native-firebase/database';
const reference = database().ref('/users/123');
```

NOTE: To get a reference to a database other than an 'us-central1' default database, you must pass the database URL. You can find your Realtime Database URL in the Realtime Database section of the Firebase console.
NOTE: To get a reference to a database other than a 'us-central1' default database, you must pass the database URL. You can find your Realtime Database URL in the Realtime Database section of the Firebase console.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Strange, I fixed this on main already, why is this showing up in a diff here? 🤔

Copy link
Member Author

@russellwheatley russellwheatley Mar 21, 2023

Choose a reason for hiding this comment

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

I updated it, I think a test was breaking because of incorrect grammar/spelling IIRC

Copy link
Member Author

Choose a reason for hiding this comment

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

Also removed the duplicated non-modular e2e test files 👍

Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks - another one bites the dust, and this was medium-large IMHO

@mikehardy mikehardy merged commit 02df92e into main Mar 23, 2023
@mikehardy mikehardy deleted the feat/storage-modular branch March 23, 2023 14:16
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.

3 participants