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

Latest release breaks Bower #26

Closed
weaverryan opened this Issue Jul 11, 2014 · 6 comments

Comments

4 participants
@weaverryan

weaverryan commented Jul 11, 2014

Hi there!

The line in question is: 1bc5c91#diff-4f99eaef47493ba13b06879592d2a1c4R251

This change appears to have broken Bower :). This is happening on my local machine AND on my Travis build - so it's not just a single-system issue. Here are the details:

  1. Update bower to the latest (to get 0.24 of this library)
  2. The bug doesn't appear on all bower deps, but here is a list you can use to recreate:
{
  "dependencies": {
    "font-awesome": "~4.0.3",
    "bootstrap-sass-official": "~3.1.1",
    "bootstrap": "~3.1.1",
    "jquery.smooth-scroll": "~1.4.13"
  }
}
  1. The error you will receive is: Arguments to path.join must be strings from GitHubResolver.js line 54 in Bower.

  2. The issue actually comes from here: https://github.com/bower/bower/blob/master/lib/core/resolvers/Resolver.js#L165

The call to tmp.dir causes dir on the next then to be an array, whereas it used to be a string. Ultimately, this causes this._tempDir to be an array, which blows up in GitHubResolver (again, seemingly only for some deps, perhaps because only some use GitHubResolver.

I'm not a node expert, so I apologize if anything is unclear or just plain wrong. But I can confirm that removing the third argument here un-breaks bower.

Thanks!

@weaverryan

This comment has been minimized.

Show comment
Hide comment
@weaverryan

weaverryan Jul 11, 2014

An issue has also been opened up on Bower about this: bower/bower#1404. Again, I think the issue is with this library (apologies if I'm wrong), so I've pointed them here.

Thanks!

weaverryan commented Jul 11, 2014

An issue has also been opened up on Bower about this: bower/bower#1404. Again, I think the issue is with this library (apologies if I'm wrong), so I've pointed them here.

Thanks!

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Jul 11, 2014

I believe that this can easily be fixed in bower, using spread.

petebacondarwin commented Jul 11, 2014

I believe that this can easily be fixed in bower, using spread.

@paulirish

This comment has been minimized.

Show comment
Hide comment
@paulirish

paulirish Jul 11, 2014

We've now fixed it in bower. 1.3.8 shipped.

Thanks for reporting this over here @weaverryan

paulirish commented Jul 11, 2014

We've now fixed it in bower. 1.3.8 shipped.

Thanks for reporting this over here @weaverryan

@raszi

This comment has been minimized.

Show comment
Hide comment
@raszi

raszi Jul 13, 2014

Owner

My aplogies, I did not think that this could break any usages and therefore it could be a safe minor upgrade.

function x(a, b) {
  console.log(a);
  console.log(b);
}
x('1', '2', '3')
1
2
undefined
Owner

raszi commented Jul 13, 2014

My aplogies, I did not think that this could break any usages and therefore it could be a safe minor upgrade.

function x(a, b) {
  console.log(a);
  console.log(b);
}
x('1', '2', '3')
1
2
undefined
@raszi

This comment has been minimized.

Show comment
Hide comment
@raszi

raszi Jul 13, 2014

Owner

As @domenic commented in #1403 it is not the fault of tmp.

Owner

raszi commented Jul 13, 2014

As @domenic commented in #1403 it is not the fault of tmp.

cscott added a commit to cscott/prfun that referenced this issue Jul 15, 2014

Tweak API of `Promise.promisify`.
Follow the lead of `denodeify` in `rsvp` and `q`: eliminate the magic
behavior of `Promise.promisify` so that adding extra arguments to a
node-style callback is not a breaking API change.  See:
raszi/node-tmp#26 (comment)
bower/bower#1403 (comment)
@raszi

This comment has been minimized.

Show comment
Hide comment
@raszi

raszi Jul 28, 2014

Owner

I think I'll close this according to @domenic comment.

Owner

raszi commented Jul 28, 2014

I think I'll close this according to @domenic comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment