-
Notifications
You must be signed in to change notification settings - Fork 2.3k
fix: implement uploadBytes for storage modular sdk #8696
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
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Can you write a test for this please? |
|
Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 15 days until this gets closed automatically |
|
Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 15 days until this gets closed automatically |
|
It is nearly impossible to write automated tests for these storage upload/download methods because they run against an in-memory emulator on same host as the client, so there is insufficient delay (even with very large files) for a test to work. I believe the only valid tests would work against cloud storage vs the emulator - which is possible now. This is the entry point for our local tests: https://github.com/invertase/react-native-firebase/blob/main/tests/local-tests/index.js And if you run |
mikehardy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code seems reasonable - I assume this is working for you @johnsimeroth ?
|
None of the CI errors are related to this PR - if you rebase to current main in this repo there are a bunch of CI fixes that should see things go green |
|
@johnsimeroth is attempting to deploy a commit to the Invertase Team on Vercel. A member of the Team first needs to authorize it. |
Apologies for the delay! Looks like I need to sort out my github notification settings, I wasn't getting any of the responses here and I honestly forgot about this PR. Yes, the code is the same that I added to my app, and it's been working as expected without issue.
That seems sensible - If you'd like to see that setup as part of this PR I can find some time next week to take a crack at it.
In the interim, I've rebased as requested. |
Description
This PR implements the uploadBytes storage function for the modular SDK, one of several unimplemented methods mentioned in #7483.
Related issues
#7483
Release Summary
Adds missing storage.uploadBytes implementation
Checklist
AndroidiOSOther(macOS, web)e2etests added or updated inpackages/\*\*/e2ejesttests added or updated inpackages/\*\*/__tests__Test Plan
No new tests, but all existing storage tests still pass. I would have mirrored any tests for uploadBytesResumable, but I didn't see any for that either.
Think
react-native-firebaseis great? Please consider supporting the project with any of the below:React Native FirebaseandInvertaseon Twitter🔥 My first PR here, LMK what you'd like to see different and I'm happy to make changes.