Skip to content
This repository has been archived by the owner on Nov 20, 2018. It is now read-only.

onProgress event should send the name parameter #1255

Closed
wants to merge 2 commits into from

Conversation

xzyfer
Copy link

@xzyfer xzyfer commented Jul 17, 2014

The onProgress event sends the name parameter as an empty string. I've tested this on 5.0.3 using the s3 client, but I expect this is broken for all clients.

This appears have broken in 74f3ca7.

I was unable to find a test that executed the function in question.


This PR also locks tmp to 0.0.23 to fix an issue with bower

The tmp package introduced a breaking change in a patch version.
This caused new bower installs to break with the following error
TypeError: Arguments to path.join must be strings.

Bower bugs for reference:
bower/bower#1399
bower/bower#1407

Unfortunately bumping bower to 1.3.8 doesn't resolve this issue
because grunt-bower-task depends on bower@1.2.x

The tmp package introduced a breaking change in a patch version.
This caused new bower installs to break with the following error
`TypeError: Arguments to path.join must be strings`.

Bower bugs for reference:
bower/bower#1399
bower/bower#1407

Unfortunately bumping bower to 1.3.8 doesn't resolve this issue
because grunt-bower-task depends on bower@1.2.x
@feltnerm
Copy link
Contributor

Thanks, @xzyfer. And the tests pass 🍻 !

I'll check this out later today. In an in-progress branch, I actually removed bower (4d92a2d) so dd83522 may not be required.

@xzyfer
Copy link
Author

xzyfer commented Jul 17, 2014

For what it's worth the patch doe bower was just to get Travis passing.

The actual bug I'm fixing is rather crippling for us and worthy or a hotfix
release IMHO.
On Jul 18, 2014 1:49 AM, "Mark Feltner" notifications@github.com wrote:

Thanks, @xzyfer https://github.com/xzyfer. And the tests pass [image:
🍻] !

I'll check this out later today. In an in-progress branch, I actually
removed bower (4d92a2d
4d92a2d)
so dd83522 dd83522 may
not be required.


Reply to this email directly or view it on GitHub
#1255 (comment).

@feltnerm
Copy link
Contributor

For what it's worth the patch doe bower was just to get Travis passing.

Right. I ran into the same issue too.

The actual bug I'm fixing is rather crippling for us and worthy or a hotfix
release IMHO.

I think you're right. I still need to verify, but once/if I do I will schedule a hotfix.

@feltnerm feltnerm added this to the 5.0.4 milestone Jul 17, 2014
@feltnerm
Copy link
Contributor

Okay I verified this in the current release. I've scheduled this for the next hotfix release. I don't have a date for the next hotfix release, but it should be soon.

thanks for the report / pull request.

@rnicholus
Copy link
Member

This issue is pretty easy to work around: simply call getName in your progress handler, passing in the ID of the file. This should be suitable until this issue is fixed in a future release.

@rnicholus
Copy link
Member

Starting work on this as part of 5.0.4.

rnicholus pushed a commit that referenced this pull request Aug 14, 2014
@rnicholus
Copy link
Member

Fixed in the hotfix/5.0.4 branch. To be part of the upcoming 5.0.4 release.

@rnicholus
Copy link
Member

@xzyfer Any interest in verifying the fix, since you reported the issue?

@xzyfer
Copy link
Author

xzyfer commented Aug 20, 2014

I can confirm this is fixed in the hotfix/5.0.4 branch

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants