Add download url support to Firebase Storage plugin #457
Conversation
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
| @@ -48,6 +48,13 @@ class StorageReference { | |||
| ); | |||
| } | |||
|
|
|||
| Future<String> getDownloadURL() async { | |||
| final dynamic downloadUrl = await FirebaseStorage._channel.invokeMethod( | |||
There was a problem hiding this comment.
I think you can make this non-async and say return FirebaseStorage._channel.invokeMethod(...
See delete for another example of this
There was a problem hiding this comment.
Done,
Is there any advantage to having the return type here by Future<String> instead of Future<dynamic>
There was a problem hiding this comment.
It's documenting that we're returning a String here and not some other type like a Uri.
| Future<String> getDownloadURL() async { | ||
| final dynamic downloadUrl = await FirebaseStorage._channel.invokeMethod( | ||
| "StorageReference#getDownloadUrl", | ||
| <String, String>{'path': _pathComponents.join("/")}); |
There was a problem hiding this comment.
If you put a trailing comma here you can avoid the double-indent
| @@ -53,6 +53,46 @@ void main() { | |||
| }); | |||
| }); | |||
|
|
|||
| group('getDownloadUrl', () { | |||
| const MethodChannel channel = const MethodChannel( | |||
There was a problem hiding this comment.
This plugin should match the other plugins in terms of the method channel name: 'plugins.flutter.io/firebase_storage'. This is can be done in a separate CL, I filed flutter/flutter#16171
|
|
||
| setUp(() { | ||
| channel.setMockMethodCallHandler((MethodCall methodCall) async { | ||
| print('get download called'); |
There was a problem hiding this comment.
No need for print statements in tests
| @@ -48,6 +48,13 @@ class StorageReference { | |||
| ); | |||
| } | |||
|
|
|||
| Future<dynamic> getDownloadURL() async { | |||
There was a problem hiding this comment.
you could make this not be async and remove the await (in addition to specifying the return type)
There was a problem hiding this comment.
I was mistaken, this is actually necessary in strong mode
There was a problem hiding this comment.
is the dynamic really needed ? Why not String ?
Remove unused `await`s
This reverts commit 6d16b63.
Add
getDownloadUrlsupport to the Firebase Storage plugin.