-
Notifications
You must be signed in to change notification settings - Fork 1.7k
docs(firestore): Add docs on Firestore x SPM integration #15387
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
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. |
/gemini review These docs are meant to educate repo maintainers on the organization of the Firestore-related targets in the Package.swift |
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 excellent documentation explaining the complex Swift Package Manager target hierarchy for Firestore. The document is well-structured and clearly explains the different build options (binary, source, and CI-local) and the testing setup. I have a couple of minor suggestions to improve consistency and clarity in the dependency diagrams and target descriptions. Overall, this is a great addition that will be very helpful for developers.
* **`FirebaseFirestoreTarget`**: A wrapper target, same as in the source-based | ||
build. |
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.
This description makes a forward reference to the "source-based build" section, which appears later in the document. It would be clearer to provide a self-contained description to improve readability.
* **`FirebaseFirestoreTarget`**: A wrapper target, same as in the source-based | |
build. | |
* **`FirebaseFirestoreTarget`**: The wrapper target that provides a stable entry point for clients. |
├── gRPC-cpp (source) (from https://github.com/grpc/grpc-ios.git) | ||
└── BoringSSL (source) (from https://github.com/firebase/boringSSL-SwiftPM.git) |
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 dependency hierarchies for binary and source builds are slightly inconsistent. The source-based build hierarchy includes BoringSSL
, which is a transitive dependency of gRPC-cpp
, while the binary-based build hierarchy appears to only list direct dependencies. To make the diagrams consistent and easier to understand, it's better to only show direct dependencies in both. This change removes the transitive BoringSSL
dependency from this diagram and updates the tree structure accordingly.
├── gRPC-cpp (source) (from https://github.com/grpc/grpc-ios.git) | |
└── BoringSSL (source) (from https://github.com/firebase/boringSSL-SwiftPM.git) | |
└── gRPC-cpp (source) (from https://github.com/grpc/grpc-ios.git) |
|
||
## 2. Source-Based Build | ||
|
||
When the `FIREBASE_SOURCE_FIRESTORE` environment variable is set, Firestore and |
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.
Should we document how to set FIREBASE_SOURCE_FIRESTORE
since, correct me if I'm wrong, this can only be set before launching Xcode. I haven't tried setting FIREBASE_SOURCE_FIRESTORE
in the Environment Variables for the scheme and then running "Reset Package Caches" but I'm fairly sure that wouldn't work.
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.
That's a great suggestion! Will do.
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.
Left some nits, but otherwise lgtm.
Is this something we'd wanna link to from other docs? Like Firestore/README.md
?
options for Firestore: from source or from a pre-compiled binary. This choice is | ||
controlled by the `FIREBASE_SOURCE_FIRESTORE` environment variable. | ||
|
||
## Main Product |
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.
nit: Why did it capitalize every word in each header?
## Main Product | |
## Main product |
|
||
## Main Product | ||
|
||
The main entry point for integrating Firestore via Swift Package Manager is the |
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.
nit: The repetition of "Swift Package Manager" throughout the docs is a bit verbose.
The main entry point for integrating Firestore via Swift Package Manager is the | |
The main entry point for integrating Firestore via SPM is the |
Changing to a comment instead of a +2, so you can enable auto merge and have @cherylEnkidu approve it instead
The Firestore bits of the Package.swift are justifiably complicated. I vibe coded these docs to explain the current state.
cc: @rafikhan
#no-changelog