-
Notifications
You must be signed in to change notification settings - Fork 126
Feat: add UseEmulator to storage api #1795
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
Summary of ChangesHello @AustinBenoit, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Firebase Storage SDK by adding support for the Firebase Storage emulator. This feature allows developers to test their applications against a local, emulated backend, which can greatly improve development speed, reduce costs associated with cloud usage, and enable offline testing scenarios. The implementation ensures that the emulator can be configured across all supported platforms before any storage operations are performed. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request adds support for using the Firebase Storage emulator across Android, iOS, and desktop platforms. The changes are well-structured, introducing a new UseEmulator
method to the public API and implementing it for each platform.
My review has identified a few issues:
- An uninitialized member variable in the desktop implementation which could lead to undefined behavior.
- The use of debug-only assertions for input validation in the iOS implementation, which would be stripped in release builds.
- A minor formatting inconsistency in the release notes.
I've provided suggestions to address these points. Overall, this is a great addition to the SDK.
NSCAssert(host && host[0] != '\0', @"Emulator host cannot be null or empty."); | ||
|
||
NSCAssert(port > 0, @"Emulator port must be a positive number."); |
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.
NSCAssert
is a debug-only assertion and is stripped from release builds. This leaves the host
and port
parameters unvalidated in release builds, which could lead to crashes or other undefined behavior if invalid values are passed. It's better to use runtime checks that are present in all build configurations to ensure robustness.
if (!(host && host[0] != '\0')) {
LogError("Emulator host cannot be null or empty.");
return;
}
if (!(port > 0)) {
LogError("Emulator port must be a positive number.");
return;
}
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
To get the url to show up correctly the storage path now depends upon the storage internal to query for the host and port
a5429d2
to
ad03abc
Compare
✅ Integration test succeeded!Requested by @AustinBenoit on commit b4de53c |
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.
LGTM, but should wait until the API review has been handled before submitting
Description
Add support for Firebase Storage emulator via
UseEmulator
.Testing
Tested this manually via unity for Desktop, Android, iOS. See the photos below:
Type of Change
Place an
x
the applicable box:Notes
Release Notes
section ofrelease_build_files/readme.md
.