-
Notifications
You must be signed in to change notification settings - Fork 17
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
Add progress events #40
Conversation
This looks pretty reasonable, I'll just need to take a closer look to make sure the API changes are ok. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this! Progress events have been on the list since the beginning and this looks like a straightforward implementation 👍
src/cljs/s3_beam/client.cljs
Outdated
: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] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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} ... ]
src/cljs/s3_beam/client.cljs
Outdated
@@ -144,7 +164,8 @@ | |||
defaults to read-string. | |||
:key-fn - a function used to generate the object key for the uploaded file on S3 | |||
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." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I think boolean parameters should also have a ?
at the end by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, document that it defaults to false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pretty good, but I've left some review notes.
src/cljs/s3_beam/client.cljs
Outdated
@@ -144,7 +164,8 @@ | |||
defaults to read-string. | |||
:key-fn - a function used to generate the object key for the uploaded file on S3 | |||
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." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I think boolean parameters should also have a ?
at the end by default.
project.clj
Outdated
@@ -7,8 +7,8 @@ | |||
|
|||
:source-paths ["src/clj" "src/cljs"] | |||
|
|||
: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"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, I set it to 1.7.170.
## Changes | ||
|
||
#### Unreleased | ||
|
||
- Add support for progress events. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 thanks for adding a changelog entry 😄.
src/cljs/s3_beam/client.cljs
Outdated
(if (contains? upload-info :error-code) | ||
(do | ||
(put! ch upload-info) | ||
(put! ch (merge {:type :error} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this just be assoc upload-info :type :error
?
src/cljs/s3_beam/client.cljs
Outdated
@@ -144,7 +164,8 @@ | |||
defaults to read-string. | |||
:key-fn - a function used to generate the object key for the uploaded file on S3 | |||
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." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, document that it defaults to false.
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... |
Paired on with @yundt, sponsored by @BillFront.
Sepearate for better reviewing, will squash the commits later. Sponsored by @BillFront.
Thanks for the review, @martinklepsch & @danielcompton. I addressed the issues, would be nice if you could have another look. |
Looking good to me 👍 Thanks for taking the time! |
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! |
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. |
@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 :) |
What do you think about this?
Paired on with @yundt