Add monitoring and management to uploads #531

pauldemarco · 2018-05-04T10:13:41Z

This PR uses similar techniques found in other firebase plugins to add management and monitoring of uploads (StorageUploadTask) to Firebase Storage.

googlebot · 2018-05-04T10:13:46Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

pauldemarco · 2018-05-04T10:15:15Z

I signed it!

googlebot · 2018-05-04T10:15:19Z

CLAs look good, thanks!

szakarias · 2018-05-07T12:31:20Z

Thank you very much for your contribution.

It is great with the file split of firebase_storage.dart. For review purposes, please do it separately in its own PR either before or after the change. Also please make sure all the tests, format, analyze etc. pass.

pauldemarco · 2018-05-07T14:21:31Z

I'll get on that real soon, thanks @szakarias!

…

On Mon, May 7, 2018, 8:31 AM Sarah Zakarias ***@***.***> wrote: Thank you very much for your contribution. It is great with the file split of firebase_storage.dart. For review purposes, please do it separately in its own PR either before or after the change. Also please make sure all the tests, format, analyze etc. pass. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#531 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AP829ncbtBJHbX81AmdEgR6HXFXzgglTks5twD6bgaJpZM4Tyac6> .

szakarias · 2018-05-09T08:34:32Z

In order to do the review, please rebase and fix the conflicts and failing tests.

szakarias

Here is the first batch of comments.

In addition, please focus this PR on the relevant change. That is, remove all the unnecessary style and format changes such as renaming, reordering, etc.

szakarias · 2018-05-15T09:27:54Z

packages/firebase_storage/lib/src/upload_task.dart

+    @required StorageReference reference,
+    StorageMetadata metadata,
+  })  : _storage = storage,
+        _reference = reference,


What is the reason for changing the argument from path to a storageReference?

Code similarity with repo.

szakarias · 2018-05-15T09:37:56Z

packages/firebase_storage/lib/src/upload_task.dart

-  final FirebaseStorage _firebaseStorage;
-  final String _path;
+  StorageUploadTask({
+    @required FirebaseStorage storage,


Why change to named parameters?

Code similarity with repo.

Style changes should be done separately.

szakarias · 2018-05-15T10:25:40Z

packages/firebase_storage/lib/src/upload_task.dart

 abstract class StorageUploadTask {
-  final FirebaseStorage _firebaseStorage;
-  final String _path;
+  StorageUploadTask({


The constructor should remain private.

szakarias · 2018-05-15T10:55:59Z

packages/firebase_storage/lib/src/upload_task.dart

+
+  /// Returns a StorageTaskSnapshot on complete, or throws error
+  Future<StorageTaskSnapshot> _start() async {
+    _handle = await _platformMethod().then<int>((dynamic result) => result);


just _handle = await _platformMethod();

szakarias · 2018-05-15T12:59:39Z

packages/firebase_storage/lib/src/upload_task.dart

+  /// Returns a StorageTaskSnapshot on complete, or throws error
+  Future<StorageTaskSnapshot> _start() async {
+    _handle = await _platformMethod().then<int>((dynamic result) => result);
+    return await _storage._methodStream


No need for await since you are returning a Future.

The multiple stream transformations makes the code hard to read. Let's try with a single where and a single map like this

final StorageTaskEvent event = await _storage._methodStream .where(...) .map((MethodCall call) {...}) .firstWhere(...); return event.snapshot;

I rather prefer the multiple .where and .map to break out the transformation steps whilst using the fat arrow notation. Is there a performance benefit to combining into code blocks?

There is definitely a performance overhead in having several transformation, but the main issue here is readability. You could also try with some helper functions that encapsulate the logic you want to express.

szakarias · 2018-05-15T20:41:41Z

packages/firebase_storage/lib/src/upload_task.dart

-      String path, StorageMetadata metadata)
-      : super._(firebaseStorage, path, metadata);
+class _StorageFileUploadTask extends StorageUploadTask {
+  _StorageFileUploadTask({


Please don't change the constructor signature unless it is functionally needed.

szakarias · 2018-05-15T20:42:32Z

packages/firebase_storage/lib/src/upload_task.dart

-      String path, StorageMetadata metadata)
-      : super._(firebaseStorage, path, metadata);
+class _StorageDataUploadTask extends StorageUploadTask {
+  _StorageDataUploadTask({


Same as above regarding constructor signature change.

szakarias · 2018-05-15T20:48:04Z

packages/firebase_storage/lib/src/upload_task.dart

-  StorageFileUploadTask._(this._file, FirebaseStorage firebaseStorage,
-      String path, StorageMetadata metadata)
-      : super._(firebaseStorage, path, metadata);
+class _StorageFileUploadTask extends StorageUploadTask {


Making this class private is a breaking change. Let's leave it public.

Are we sure we want to do this? This will introduce some ambiguity for the end user. Why expose StorageFileUploadTask and StorageDataUploadTask when the only functionality that would be needed is found in the base class, StorageUploadTask?

If we decide on this, it should be done in a separate PR.

szakarias · 2018-05-15T20:48:39Z

packages/firebase_storage/lib/src/upload_task.dart

-  StorageDataUploadTask._(this._bytes, FirebaseStorage firebaseStorage,
-      String path, StorageMetadata metadata)
-      : super._(firebaseStorage, path, metadata);
+class _StorageDataUploadTask extends StorageUploadTask {


Let's keep this public too.

szakarias · 2018-05-15T20:49:35Z

packages/firebase_storage/pubspec.yaml

@@ -3,7 +3,7 @@ description: Flutter plugin for Firebase Cloud Storage, a powerful, simple, and
  cost-effective object storage service for Android and iOS.
 author: Flutter Team <[email protected]>
 homepage: https://github.com/flutter/plugins/tree/master/packages/firebase_storage
-version: 0.3.4
+version: 0.3.5


If it ends up being a breaking change, increment the minor version instead.

Hixie · 2018-05-29T22:40:05Z

@pauldemarco Will you be able to continue working on this? It's totally fine if not; we can take it over (though it might take longer). I'm just asking because I'm trying to clean out our review queue.

pauldemarco · 2018-05-29T22:55:04Z

Hi @Hixie, yep i've got some time carved out later this week for this.

pauldemarco · 2018-05-29T23:03:26Z

@szakarias Many of those formatting and internal changes were to increase code similarity between other plugins in this repo, like FirebaseDatabase and CloudFirestore.

I'll take a look at what might be unnecessary and revert, as well as apply those other requests later this week.

Thanks!

szakarias · 2018-05-31T12:07:02Z

packages/firebase_storage/lib/src/upload_task.dart

-  StorageFileUploadTask._(this._file, FirebaseStorage firebaseStorage,
-      String path, StorageMetadata metadata)
-      : super._(firebaseStorage, path, metadata);
+class _StorageFileUploadTask extends StorageUploadTask {


If we decide on this, it should be done in a separate PR.

szakarias · 2018-05-31T12:09:26Z

packages/firebase_storage/lib/src/upload_task.dart

+  /// Returns a StorageTaskSnapshot on complete, or throws error
+  Future<StorageTaskSnapshot> _start() async {
+    _handle = await _platformMethod().then<int>((dynamic result) => result);
+    return await _storage._methodStream


There is definitely a performance overhead in having several transformation, but the main issue here is readability. You could also try with some helper functions that encapsulate the logic you want to express.

szakarias · 2018-05-31T12:22:24Z

packages/firebase_storage/lib/src/upload_task.dart

-  final FirebaseStorage _firebaseStorage;
-  final String _path;
+  StorageUploadTask({
+    @required FirebaseStorage storage,


Style changes should be done separately.

szakarias · 2018-05-31T12:26:04Z

packages/firebase_storage/lib/src/upload_task.dart

-// found in the LICENSE file.
+// Copyright 2017, the Flutter project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.


Our license header should say Chromium Authors

pauldemarco · 2018-06-04T03:27:46Z

@szakarias I reverted a bunch of styling changes, and applied the changes for _setState.

It's unclear whether it's the PR author's responsibility to update the CHANGELOG and pubspec version, so I left it alone for now.

Let me know of anything else.

Thanks!

szakarias · 2018-06-07T19:48:09Z

Thanks for your update.

I can see you removed the example code. Could you update the example again to exercise some of the new features?

Also going forward, please don't squash commits as this makes reviewing harder.

Hixie · 2018-06-26T23:55:42Z

@pauldemarco Please let us know if you be able to continue working on this. If you don't have an opportunity to do so that's totally fine, we will in due course fix the remaining issues and land the patch. Thanks for your contributions so far!

pauldemarco · 2018-06-30T23:02:37Z

@Hixie @szakarias I'll be able to include a cleaned up example this week, thanks for your patience.

…sk is null.

…s null.

This reverts commit 1a9d099

…roid API.

googlebot · 2018-08-25T00:12:12Z

CLAs look good, thanks!

kroikie

@pauldemarco Once you remove the unnecessary 'CANCELLED' print, I think this is good to merge.

kroikie · 2018-08-27T22:25:05Z

packages/firebase_storage/lib/src/upload_task.dart

+        break;
+      case StorageTaskEventType.failure:
+        isComplete = true;
+        print('CANCELLED');


I think we can remove this print statement.

* Upload monitoring and management added for Android and iOS.

This reverts commit 995967c.

* Upload monitoring and management added for Android and iOS.

googlebot added the cla: no label May 4, 2018

googlebot added cla: yes and removed cla: no labels May 4, 2018

mravn-google assigned szakarias May 4, 2018

pauldemarco force-pushed the firebase-storage-uploadtask branch from 41ea484 to c151dd5 Compare May 4, 2018 20:58

pauldemarco mentioned this pull request May 7, 2018

Split files into library structure. #550

Merged

pauldemarco force-pushed the firebase-storage-uploadtask branch from b7d8155 to a808b4b Compare May 9, 2018 15:51

szakarias suggested changes May 15, 2018

View reviewed changes

szakarias reviewed May 31, 2018

View reviewed changes

pauldemarco force-pushed the firebase-storage-uploadtask branch 4 times, most recently from 674dc7a to c0d4317 Compare June 4, 2018 03:23

kroikie added the flutterfire label Jun 19, 2018

Hixie added the needs love label Jun 26, 2018

pauldemarco force-pushed the firebase-storage-uploadtask branch 2 times, most recently from a6da37d to dd52c7a Compare July 3, 2018 18:16

pauldemarco added 17 commits August 24, 2018 17:07

_methodStreamController now static to avoid multiple instances.

6387430

StorageUploadTask _platformMethod -> _platformStart to avoid ambiguity.

5d3c9a6

StorageUploadTask _setState -> _changeState to avoid ambiguity.

59c5da6

[Android] Pause, resume, and cancel return errors if platform-side ta…

aa5debb

…sk is null.

kProgress -> kResume.

51f13b0

[iOS] Pause, resume, and cancel return errors if platform-side task i…

7a5c0cf

…s null.

Formatting fixes.

cf8b2b5

Revert "Copyright 2017 -> Copyright 2018"

a29e9c0

This reverts commit 1a9d099

Updated CHANGELOG and pubspec.yaml version.

acd7a14

Removed downloadUrl from StorageUploadTask and StorageTaskSnapshot.

63f588b

StorageTaskSnapshot has ref field for getDownloadUrl. Matches iOS/And…

491a9d2

…roid API.

Specified type annotations.

8fb44cc

Removed old API call to getDownloadUrl, bumped version.

5b9e6eb

Fixed imports and formatting issue.

5f4447f

Formatting fix.

99bfe91

Formatting fix.

22df07d

kroikie force-pushed the firebase-storage-uploadtask branch from ec250fe to 22df07d Compare August 25, 2018 00:12

googlebot added cla: yes and removed cla: no labels Aug 25, 2018

fix analyze errors

d9ad334

kroikie approved these changes Aug 27, 2018

View reviewed changes

Remove unnecessary print statement

0a4d532

kroikie merged commit cd60fb0 into flutter:master Sep 4, 2018

nichtverstehen pushed a commit to nichtverstehen/plugins that referenced this pull request Sep 6, 2018

Add monitoring and management to uploads (flutter#531)

adb3cd0

* Upload monitoring and management added for Android and iOS.

Hixie removed the needs love label Oct 24, 2018

andreidiaconu pushed a commit to andreidiaconu/plugins that referenced this pull request Feb 17, 2019

Add monitoring and management to uploads (flutter#531)

995967c

* Upload monitoring and management added for Android and iOS.

andreidiaconu added a commit to andreidiaconu/plugins that referenced this pull request Feb 17, 2019

Revert "Add monitoring and management to uploads (flutter#531)"

4604010

This reverts commit 995967c.

julianscheel pushed a commit to jusst-engineering/plugins that referenced this pull request Mar 11, 2020

Add monitoring and management to uploads (flutter#531)

83b9a81

* Upload monitoring and management added for Android and iOS.

Add monitoring and management to uploads #531

Add monitoring and management to uploads #531

Conversation

pauldemarco commented May 4, 2018

googlebot commented May 4, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

pauldemarco commented May 4, 2018

googlebot commented May 4, 2018

szakarias commented May 7, 2018

pauldemarco commented May 7, 2018 via email

szakarias commented May 9, 2018

szakarias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hixie commented May 29, 2018

pauldemarco commented May 29, 2018 • edited Loading

pauldemarco commented May 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pauldemarco commented Jun 4, 2018

szakarias commented Jun 7, 2018

Hixie commented Jun 26, 2018

pauldemarco commented Jun 30, 2018

googlebot commented Aug 25, 2018

kroikie left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pauldemarco commented May 29, 2018 •

edited

Loading

kroikie left a comment •

edited

Loading