Add progress events by iGEL · Pull Request #40 · martinklepsch/s3-beam

iGEL · 2017-03-01T13:20:13Z

What do you think about this?

Paired on with @yundt

danielcompton · 2017-03-02T00:23:22Z

This looks pretty reasonable, I'll just need to take a closer look to make sure the API changes are ok. Thanks!

martinklepsch

Thanks for this! Progress events have been on the list since the beginning and this looks like a straightforward implementation 👍

martinklepsch · 2017-03-04T05:38:45Z

src/cljs/s3_beam/client.cljs

+    :bytes-sent - the size of the already uploaded potion in bytes
+    :bytes-total - the total file size in bytes
+    :identifier - an identifier pass-through value for caller to identify the file with"
+  [progress-events {:keys [f identifier form-data upload-url] :as upload-info} ch]


I think instead of passing progress-events as argument here we should pass a map so we can extend the number of options passed without introducing backwards incompatible (fn-arity) changes.

[{:keys [progress-events] :as opts} ... ]

martinklepsch · 2017-03-04T05:40:56Z

src/cljs/s3_beam/client.cljs

                       defaults to nil, which means it will use the passed filename as the object key.
-    :headers-fn      - a function used to create the headers for the GET request to the signing server."
+    :headers-fn      - a function used to create the headers for the GET request to the signing server.
+    :progress-events - if true, progress events will be pushed on to channel during upload."


I wonder if this should maybe be :progress-events? since it's a boolean flag? Or do we only do that with functions that return boolean values?

Yep, I think boolean parameters should also have a ? at the end by default.

Also, document that it defaults to false.

danielcompton

This is pretty good, but I've left some review notes.

danielcompton · 2017-03-06T00:08:11Z

src/cljs/s3_beam/client.cljs

                       defaults to nil, which means it will use the passed filename as the object key.
-    :headers-fn      - a function used to create the headers for the GET request to the signing server."
+    :headers-fn      - a function used to create the headers for the GET request to the signing server.
+    :progress-events - if true, progress events will be pushed on to channel during upload."


Yep, I think boolean parameters should also have a ? at the end by default.

danielcompton · 2017-03-06T00:08:54Z

project.clj


-  :dependencies [[org.clojure/clojure "1.6.0" :scope "provided"]
-                 [org.clojure/clojurescript "0.0-2371" :scope "provided"]
+  :dependencies [[org.clojure/clojure "1.8.0" :scope "provided"]


Do these upgrades need to be done as part of this change? If they're separate it can make it easier to bisect any breakage down the line.

I guess, I could revert the clojure bump.

About cljs: Google's XhrIo lib got support for these events only in August 2015, so I assume it's in the v20150901 release (there is no changelog entry). Clojurescript bumped the version in October 2015, which was included in the 1.7.170 release.

I'm not that experienced with clojurescript, so I trust you which version I should require here. I'm not even sure, whether this has effects on the users of the lib. 😉

For now, I set it to 1.7.170.

danielcompton · 2017-03-06T00:09:48Z

README.md


 #### Unreleased

+- Add support for progress events.


👍 thanks for adding a changelog entry 😄.

danielcompton · 2017-03-06T00:11:08Z

src/cljs/s3_beam/client.cljs

  (if (contains? upload-info :error-code)
    (do
-      (put! ch upload-info)
+      (put! ch (merge {:type :error}


Should this just be assoc upload-info :type :error?

danielcompton · 2017-03-06T00:11:56Z

src/cljs/s3_beam/client.cljs

                       defaults to nil, which means it will use the passed filename as the object key.
-    :headers-fn      - a function used to create the headers for the GET request to the signing server."
+    :headers-fn      - a function used to create the headers for the GET request to the signing server.
+    :progress-events - if true, progress events will be pushed on to channel during upload."


Also, document that it defaults to false.

danielcompton · 2017-03-06T00:13:57Z

I wish GitHub's review system didn't feel so negative when I requested changes. It makes me feel like I've put a big red x all over it, when that's not how I feel at all...

@yundt

Paired on with @yundt, sponsored by @BillFront.

Sepearate for better reviewing, will squash the commits later. Sponsored by @BillFront.

iGEL · 2017-03-06T10:32:24Z

Thanks for the review, @martinklepsch & @danielcompton. I addressed the issues, would be nice if you could have another look.

martinklepsch · 2017-03-06T13:20:05Z

Looking good to me 👍 Thanks for taking the time!

danielcompton · 2017-03-06T19:41:02Z

I've pushed an alpha3 version to GitHub, @martinklepsch are you able to deploy to Clojars? @iGEL once it's all deployed and you're using it in production, can you report back in a week with any progress? (ba dum tish)

If it's all good then we can release an 0.6.0. Thanks!

iGEL · 2017-03-06T20:46:07Z

Thanks for the support and merge!

Unfortunately we're still a few weeks away from production usage. Also, we have very few (but valuable) users, a proper cross browser test will be more telling. Hope that someone has a better test field.

martinklepsch · 2017-03-08T15:37:00Z

@danielcompton @iGEL Hey folks, I just pushed to Clojars:

[org.martinklepsch/s3-beam "0.6.0-alpha3"]

Sorry for the delay on this and nice work everyone :)

danielcompton self-requested a review March 2, 2017 00:23

danielcompton assigned iGEL and danielcompton Mar 2, 2017

martinklepsch reviewed Mar 4, 2017

View reviewed changes

danielcompton requested changes Mar 6, 2017

View reviewed changes

iGEL added 2 commits March 6, 2017 11:26

Add progress events

2761970

Paired on with @yundt, sponsored by @BillFront.

Review comments

9126958

Sepearate for better reviewing, will squash the commits later. Sponsored by @BillFront.

iGEL force-pushed the progress-events branch from 29c4541 to 9126958 Compare March 6, 2017 10:27

danielcompton approved these changes Mar 6, 2017

View reviewed changes

danielcompton merged commit c7b0edd into martinklepsch:master Mar 6, 2017

iGEL deleted the progress-events branch March 6, 2017 20:46

Conversation

iGEL commented Mar 1, 2017

Uh oh!

danielcompton commented Mar 2, 2017

Uh oh!

martinklepsch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielcompton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielcompton commented Mar 6, 2017

Uh oh!

iGEL commented Mar 6, 2017

Uh oh!

martinklepsch commented Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danielcompton commented Mar 6, 2017

Uh oh!

iGEL commented Mar 6, 2017

Uh oh!

martinklepsch commented Mar 8, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

martinklepsch commented Mar 6, 2017 •

edited

Loading