[go: nahoru, domu]

Skip to content
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

[FR]: Async Await FirebaseStorage observers #10574

Closed
Faktorealchik opened this issue Dec 14, 2022 · 8 comments · Fixed by #11289
Closed

[FR]: Async Await FirebaseStorage observers #10574

Faktorealchik opened this issue Dec 14, 2022 · 8 comments · Fixed by #11289

Comments

@Faktorealchik
Copy link
Faktorealchik commented Dec 14, 2022

Description

https://github.com/firebase/firebase-ios-sdk/blob/master/FirebaseStorage/Sources/AsyncAwait.swift

In this file we emit StorageUploadTask, and we can't add observers, so we don't know about the progress. (e.g. putFileAsync)

Uploading large files difficult to handle without knowing status

API Proposal

Perhaps you can add delegate and handle all observers inside, then tell that something is changed to delegate?

Firebase Product(s)

Storage

@google-oss-bot
Copy link

I couldn't figure out how to label this issue, so I've labeled it for a human to triage. Hang tight.

@paulb777
Copy link
Member

Thanks for the request. We're exploring an async/await API with Progress support in #10161. We're somewhat hesitant to go forward with this since Apple's roadmap for Progress is unclear. See https://forums.swift.org/t/what-s-next-for-foundation/61939#removed-types-8

In the meantime, the original non-async/await Storage APIs continue to support Progress.

@Faktorealchik
Copy link
Author

Hello @paulb777

Thank you!

@paulb777
Copy link
Member

After more thought, I believe the original APIs that return Task handles should be sufficient if you want to check task status. The async/await API's should only be used for fire and forget mode.

I'll close here, but feel free to continue the conversation.

@Faktorealchik
Copy link
Author

@paulb777 Hi, yeah, I'm using original Api to check statuses now, but I want to improve my code to fit async await, and functions like delete are working well, so I thought if you can improve other methods as well, so I won't loose any functionality

@Faktorealchik
Copy link
Author

@paulb777 Hi

Why do you think that async await API should be different from the original one?
From my perspective it should be the same. Also Apple's doing the same thing. For example you can access 'fractionCompleted' in async/await in Progress, it is easy to access via KVO. What do you think about KVO?

@paulb777 paulb777 reopened this Jan 3, 2023
@JarnoRFB
Copy link

I defined an extension which takes a closure, which is called when progress happens, while the result is still returned from an async function.

extension StorageReference {
    func write(toFile url: URL, onProgress: @escaping (Double?) -> Void) async throws {
        let downloadTask = self.write(toFile: url)
        try await withCheckedThrowingContinuation { continuation in
            downloadTask.observe(.progress) {
                onProgress($0.progress?.fractionCompleted)
            }
            
            downloadTask.observe(.success) { _ in
                continuation.resume(with: .success(()))
            }
            
            downloadTask.observe(.failure) { snapshot in
                continuation.resume(with: .failure(snapshot.error ?? NSError()))
            }
        }
    }
}

// on the call site
try await reference .write(toFile: file) { progress in
    print(progress ?? 0.0)
}

That is working fine so far, but an integration of something like this into the package would be nice.

@paulb777
Copy link
Member

@JarnoRFB Thanks for the suggestion! PR in progress at #11289

@firebase firebase locked and limited conversation to collaborators Jun 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants