-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Firestore Swift Cpp Experiment #13956
Open
cherylEnkidu
wants to merge
36
commits into
main
Choose a base branch
from
cheryllin/firestoreSwiftCpp
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Generated by 🚫 Danger |
Apple API Diff ReportCommit: ca0161f FirebaseCoreFunctions[MODIFIED] FIRFirebaseVersion[MODIFIED] FIRFirebaseVersionSwift:
+ func FirebaseVersion () -> String
- func FirebaseVersion () -> String
Objective-C:
+ NS_SWIFT_NAME ( FirebaseVersion ()) NSString * FIRFirebaseVersion ( void )
- NSString * _Nonnull FIRFirebaseVersion ( void ) [BUILD ERROR] FirebaseFirestoreInternal[BUILD ERROR] FirebaseFirestore |
cherylEnkidu
force-pushed
the
cheryllin/firestoreSwiftCpp
branch
from
October 24, 2024 18:43
4a5f5fd
to
7c7ac2e
Compare
paulb777
reviewed
Oct 29, 2024
FirebaseFirestore.podspec
Outdated
s.public_header_files = 'FirebaseFirestoreInternal/**/*.h' | ||
s.public_header_files = [ | ||
'FirebaseFirestoreInternal/**/*.h', | ||
'Firestore/Swift/Source/SwiftAPI/*.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.
Swift files should not be in the headers
cherylEnkidu
force-pushed
the
cheryllin/firestoreSwiftCpp
branch
from
November 1, 2024 20:25
4af3d5b
to
497cbf8
Compare
cherylEnkidu
force-pushed
the
cheryllin/firestoreSwiftCpp
branch
from
January 27, 2025 19:07
be13f5d
to
504ee98
Compare
cherylEnkidu
force-pushed
the
cheryllin/firestoreSwiftCpp
branch
from
January 27, 2025 20:10
504ee98
to
fe4ec87
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Here are some imperfections in this PR. Please feel free to suggest better solutions:
Exposure of C++ APIs in the Objective-C Interface
The Swift layer can only access public APIs of Objective-C. In Swift Package Manager, only a single path can be designated as the public header path. To enable additional features in the Swift layer, it must access internal C++ members held by the Objective-C object.
Swift's async/await functionality requires Objective-C callbacks for implementation. As a result, a public API—used exclusively by the Swift layer—was introduced. (Potentially this can be avoid using a C++ class contains Swift member, since the current structure only supports one way dependency, Swift class has C++ member. This impl is listed as a future improvement)
The FirebaseFirestoreCpp binary build structure in Swift Package Manager is currently written as pseudo-code and has not been verified.
The include path in this PR using relative path instead of full path, since full path leads to build failure for unknown reason.