slushjs / gulp-install

Automatically install npm and bower packages if package.json or bower.json is found in the gulp file stream respectively
MIT License
106 stars 46 forks source link

Add CWD parameter to child process #2

Closed nanymor closed 10 years ago

nanymor commented 10 years ago

Install process seems to fail when packages are located in nested directories. Added cwd parameter to spawn the child process from the directory where the Package.json and bower.json are located.

joakimbeng commented 10 years ago

Sounds like a good idea. Though I think setting cwd to path.dirname(file.path) is wrong, because that will point to the file's source location, not its destination. I think it should be something like: path.dirname(path.resolve(process.cwd(), file.relative)) instead, what do you think?

nanymor commented 10 years ago

You have a point there, your solution seems more robust. I wonder how it's working as it is right now; might it be that the files in the stream have gone through gulp.dest() and so file.path returns the new path already?

joakimbeng commented 10 years ago

Actually I think your solution might be more robust still though. As you say it relies on the fact that the files are piped through install() after gulp.dest() and that will always give the expected result. My solution would have to take the destination directory as a parameter (if it's something else than process.cwd()) to work completely, like gulp-conflict does. And maybe that's the best solution because then it will not depend on any other plugins nor the order in which the plugins are piped.

The code would then be: path.dirname(path.resolve(process.cwd(), dest, file.relative))

What do you think?

mechanoid commented 10 years ago

Hey, while i missed this PR, i pushed a much simpler and probably a much more stupid solution as a PR for my own :). So feel free to ignore my solution :)

nanymor commented 10 years ago

Sorry for the late reply. I think the plugin should be kept as simple as possible, so In my opinion an additional parameter is not needed. The plugin should just process a stream as it comes in, so in the context of a slushfile it does rely on the fact that the files are piped through a dist() task, and in all other cases it should just work fine. Anyway, your call, and thanks again for building slush!

joakimbeng commented 10 years ago

@nanymor I will go with your PR. I think that's the best solution, as you said it should be kept as simple as possible. Therefor it should only rely on file.path like your solution does. Thank you for your contribution :)