martinklepsch / s3-beam

🚀 direct-to-S3 uploading using ClojureScript
Eclipse Public License 1.0
92 stars 17 forks source link

upload progress #15

Closed GetContented closed 8 months ago

GetContented commented 8 years ago

Is upload progress still on the cards?

martinklepsch commented 8 years ago

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.

GetContented commented 8 years ago

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.

martinklepsch commented 8 years ago

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

GetContented commented 8 years ago

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 commented 8 years ago

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.

martinklepsch commented 8 years ago

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.

GetContented commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

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

martinklepsch commented 8 years ago

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

GetContented commented 8 years ago

Cool. :)

martinklepsch commented 8 years ago

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.

GetContented commented 8 years ago

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.

kennyjwilli commented 8 years ago

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

GetContented commented 8 years ago

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 ? :)

bensu commented 8 years ago

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.

GetContented commented 8 years ago

@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.

bensu commented 8 years ago

@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!

GetContented commented 8 years ago

@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. :)

martinklepsch commented 8 years ago

@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 commented 8 years ago

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

altV commented 8 years ago

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

GetContented commented 8 years ago

@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.