s3-beam icon indicating copy to clipboard operation
s3-beam copied to clipboard

upload progress

Open GetContented opened this issue 9 years ago • 23 comments

Is upload progress still on the cards?

GetContented avatar Jul 26 '15 14:07 GetContented

Still would be nice to have but I currently don't use s3-beam for anything that would require it. If you'd like to tackle it I'd be happy to discuss options, review code etc.

martinklepsch avatar Jul 26 '15 15:07 martinklepsch

Ah ok. Well I'm going to need to use it soon (implementing direct uploads in my app, and would like progress), so I might have a look at it. :) thanks. I'll close this until then.

GetContented avatar Jul 26 '15 15:07 GetContented

Alright. Feel free to ping me if you have any questions.

martinklepsch avatar Jul 26 '15 15:07 martinklepsch

I'm just going to leave a reference to this here for later: https://github.com/google/closure-library/issues/465 so that I can watch it, because it's pertinent.

GetContented avatar Jul 28 '15 15:07 GetContented

Cool, @martinklepsch looks like progress events are now supported in Google Closure library as at https://github.com/google/closure-library/commit/1e0c8103ae97a0e568e63bf5d6906597b25a4544 which is August 26, 2015 (yesterday). I'm pretty sure the clojurescript library doesn't include the very latest closure library, so I guess we wait until David Nolan rolls this into CLJS main and releases another patch/update so we can bump the required dependencies.

Current CLJS is 1.7.48 as you no doubt know but not sure what version that tracks... the tagged version after the prerelease (1.7.58) seems to be 1.7.107 (given you're a contributor to it), which tracks 0.0-20150805-acd8b553 which is still about 22 days ago... any chance you can help this along, some? :)

I don't know the machinations of what's proper and possible here. I also don't know how google release their closure code, seeing as their github doesn't seem to track releases, only commits.

GetContented avatar Aug 27 '15 12:08 GetContented

Awesome! I think updating the Closure library that comes with CLJS is a matter of bugging @swannodette to do so. AFAIK there's not much work to it so it shouldn't take too long.

There are a bunch of scripts in the Clojurescript repo to help with this as well in case you want to play around locally.

martinklepsch avatar Aug 27 '15 12:08 martinklepsch

I'm not entirely sure how to bug @swannodette actually... do you think he'll notice if we mention him here? I don't know if that's how github works.

@swannodette if you do notice this, would you mind terribly updating clojuresrcipt to bump the version of Google Closure library to include the new upload progress work?

GetContented avatar Aug 28 '15 08:08 GetContented

@martinklepsch Does github notify david when I mention him here? I can't actually open an issue on the clojurescript repo because they're not using the issue tracker.

GetContented avatar Sep 03 '15 11:09 GetContented

Oh... scratch that. Seems he bumped it 26 minutes ago! :) Great now I just have to wait for him to do a release and then get you to bump the CLJS version :). https://github.com/clojure/clojurescript/commit/1b8b4929c47b4d875d2b1c143f25dbfb462138c6

GetContented avatar Sep 03 '15 11:09 GetContented

Actually that's just the compiler but I think he'll bump the lib as well now :)

martinklepsch avatar Sep 03 '15 11:09 martinklepsch

Cool. :)

GetContented avatar Sep 03 '15 11:09 GetContented

Watch this for a new release: http://search.maven.org/#search%7Cga%7C1%7Cg%3A%22org.clojure%22%20closure

If you then specify those libs as explicit deps they will be used instead of CLJS default ones. Alternatively wait until later today for a new CLJS release with updated closure libs.

martinklepsch avatar Sep 03 '15 12:09 martinklepsch

No hurry. Sorry for the noise. I won't get to this for at least a day or two, and possibly no sense starting this until the previous PR is accepted, as it'd need error reporting to some degree, too. :) Awesome! Thanks.

GetContented avatar Sep 03 '15 12:09 GetContented

Status report on this? @GetContented have you started work on this?

kennyjwilli avatar Nov 24 '15 03:11 kennyjwilli

Hey Kenny. My last PR 21 wasn't accepted yet. I don't know why. It was requisite to this work. However, I didn't actually got to doing upload progress because of the requisites not being in place in the Google closure and CLJS requirements.

I stopped work on the clojurescript version of the project I'm working on a few months back so I probably won't implement this. I don't think it's too much work, though, on top of PR-21. If you need it, you'll probably need PR-21 to be accepted or something that does the same work. @bensu tried to get a PR of a similar nature in once before around similar lines. I'm not entirely sure why @martinklepsch didn't accept PR-21. Maybe we could ask him ? :)

GetContented avatar Nov 24 '15 03:11 GetContented

It's not that @martinklepsch didn't accept my PR, but it is incomplete. I am not currently using the project but my circle around to finish it anyway.

bensu avatar Nov 24 '15 09:11 bensu

@bensu fair enough. Mine is complete, but I think it's slightly in conflict with yours as I went about things in a slightly different manner... mine is a little bigger in terms of features, though, because I was building it with a view to upload progress later.

GetContented avatar Nov 24 '15 10:11 GetContented

@GetContented if you feel yours will be better, I won't push mine further/deal with the conflicts later. Go ahead and continue to work ignoring my PR!

bensu avatar Nov 24 '15 15:11 bensu

@bensu I have no idea - yours didn't do what I needed, and I've been using my PR since I put it up here, but the fact that it hasn't been accepted is a bit curious to me. Not sure why. :)

GetContented avatar Nov 24 '15 15:11 GetContented

@GetContented I think I didn't merge this back then because we were still waiting for a new version of the Closure Library getting bundled with ClojureScript.

It seems that either 1.7.122 or maybe even 1.7.145 is required. Adding a note to the Readme about the version requirements of s3-beam would probably be a good idea given these releases are fairly. recent.

As for #21 I'll reply there.

martinklepsch avatar Nov 24 '15 19:11 martinklepsch

Ahem.. I got confused. The new release is only required for the progress stuff which is TBD.

martinklepsch avatar Nov 24 '15 19:11 martinklepsch

any update on this issue? which direction should I look at if I want to implement this feature? (cljs newcomer)

altV avatar Mar 12 '16 08:03 altV

@altV my project didn't end up requiring it (because I started to rewrite the next version of it in Haskell).

This isn't a huge amount of work, though, I don't think. I did the precursor stuff that was required. You just need to integrate Google's closure library (with an S) upload progress.

GetContented avatar Mar 12 '16 11:03 GetContented