search

gulpjs / glob-stream

Readable streamx interface over anymatch.
MIT License
178 stars 53 forks source link

Release 5.3.3 introduce bug on Windows with base Path #68

Closed danivek closed 8 years ago

danivek commented 8 years ago

Hi,

Before release 5.3.3 and this commit https://github.com/gulpjs/glob-stream/commit/4a01f00b8bf991c892e77c4635884e1eb30705fc, basePath ended with a path.sep. And now this behavior has moved to the getBasePath function which lost the path.sep.

On Windows, many gulp plugin not work anymore since this release.

phated commented 8 years ago

@doowb any thoughts on this?

phated commented 8 years ago

I'm guessing resolveGlob is removing the separator that is added?

doowb commented 8 years ago

I'm looking into this and enabled appveyor on my fork to test out a solution.

resolveGlob will keep the trailing slash if it's already there but it only handles /. I'm not sure if this should be handled in resolveGlob or if it should be done here.

I think it should be done here because when using an absolute path and the root option, the glob parent before being expanded is turning into /. I think it's just that case that needs to be handled.

@phated will you enable appveyor for this repo... I've added an appveyor config file on my fork and I can submit a PR when it's fixed and we'll be able to see the tests in that PR.

Oh... and I had to change the jscs pattern in the package.json from *.js to . for it to work on windows.

phated commented 8 years ago

I had been meaning to turn on appveyor for this project but was going to do it when I normalized the project. I've gone ahead and enabled it now.

doowb commented 8 years ago

Just did a PR. It handles the different cases that I ran across when trying different implementations. Since the tests are already using path.sep they're valid for all environments.

danivek commented 8 years ago

I test the PR and it works fine for me

phated commented 8 years ago

Fix published as 5.3.4

phated commented 7 years ago

@danivek As I've been working on the next major release of glob-stream, it seems to make sense to remove the trailing separator on the base property. We are already going to be removing it in Vinyl, so it seems counterintuitive to include it here and then remove it again. Do you know what plugins broke with this change before? I'd like to review them and see if they aren't using path.join or other path.* utilities like is typically expected.

danivek commented 7 years ago

@phated the plugin having the issue was gulp-angular-templatecache

phated commented 7 years ago

@danivek thanks! It seems like they are properly handling separators or no-separators in the base property but I'm not sure why they are doing some of the things. Unfortunately, that module is breaking the plugin guidelines by combining other plugins internally, so I'd recommend moving away from it.

danivek commented 7 years ago

@phated thanks for your advice. Never mind, i will switch to a more stable tool webpack