Merge FirebaseStorageSwift and FirebaseStorage #9045
Merged
Conversation
This file contains hidden or 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 |
paulb777
commented
Dec 8, 2021
| list(maxResults: maxResults, | ||
| pageToken: pageToken, | ||
| completion: getResultCallback(completion: completion)) | ||
| __list(withMaxResults: maxResults, |
There was a problem hiding this comment.
list implementation for Swift clients that will find the error handling in Result.swift
| (void (^)(FIRStorageListResult *result, NSError *_Nullable error))completion | ||
| NS_SWIFT_NAME(list(maxResults:completion:)); | ||
| completion:(void (^)(FIRStorageListResult *result, | ||
| NSError *_Nullable error))completion NS_REFINED_FOR_SWIFT; |
| @@ -12,7 +12,7 @@ | |||
| // See the License for the specific language governing permissions and | |||
| // limitations under the License. | |||
|
|
|||
| import FirebaseStorage | |||
| @_exported import FirebaseStorageObjC | |||
There was a problem hiding this comment.
Pass Objective C APIS through to Swift.
ncooke3
approved these changes
Dec 8, 2021
FirebaseCombineSwift/Tests/Unit/Storage/StorageReferenceTests.swift
Outdated
Show resolved
Hide resolved
FirebaseStorage/Sources/Public/FirebaseStorage/FIRStorageReference.h
Outdated
Show resolved
Hide resolved
FirebaseStorage/Sources/Public/FirebaseStorage/FIRStorageReference.h
Outdated
Show resolved
Hide resolved
|
Going to merge this here to storage-v9 since this now demonstrates an always Swift-on top library approach along with providing the structure to pass through and/or override any Objective C API. I'll address open questions and next steps in subsequent PRs. Feel free to continue to add comments and I'll address those as well. |
paulb777
added a commit
that referenced
this pull request
Dec 8, 2021
paulb777
added a commit
that referenced
this pull request
Jan 4, 2022
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
We originally thought that we could move to combined Swift/ObjC libraries with a single combined CocoaPod and a dependent SwiftPM library. However, that strategy is problematic since it makes usability of public imports different between the two package managers, along with testing and usability complications.
This PR prototypes using the same library structure for both. It also solves the problem of providing Swift error codes for any API that needs them by using
REFINED_FOR_SWIFTfor any applicable API and wrapping the error handling in a Swift specific API. See thelistAPI implementation.I believe we have two unavoidable breakages:
Open Questions:
@_exported import FirebaseStorageObjCto pass through Objective C APIs or should we explicitly wrap all of them in Swift after marking them asREFINED_FOR_SWIFT?can_importto continue to supportimport Firebase?Next Steps for Storage:
list