Skip to content
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

Release 5.3.3 introduce bug on Windows with base Path #68

Closed
danivek opened this issue Aug 25, 2016 · 11 comments · Fixed by #69
Closed

Release 5.3.3 introduce bug on Windows with base Path #68

danivek opened this issue Aug 25, 2016 · 11 comments · Fixed by #69

Comments

@danivek
Copy link
Contributor

danivek commented Aug 25, 2016

Hi,

Before release 5.3.3 and this commit 4a01f00, 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
Copy link
Member

phated commented Aug 25, 2016

@doowb any thoughts on this?

@phated
Copy link
Member

phated commented Aug 25, 2016

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

@doowb
Copy link
Member

doowb commented Aug 25, 2016

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
Copy link
Member

phated commented Aug 25, 2016

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
Copy link
Member

doowb commented Aug 25, 2016

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
Copy link
Contributor Author

danivek commented Aug 26, 2016

I test the PR and it works fine for me

@phated
Copy link
Member

phated commented Aug 26, 2016

Fix published as 5.3.4

@phated
Copy link
Member

phated commented Oct 14, 2016

@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
Copy link
Contributor Author

danivek commented Oct 16, 2016

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

@phated
Copy link
Member

phated commented Oct 17, 2016

@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
Copy link
Contributor Author

danivek commented Oct 17, 2016

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

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

Successfully merging a pull request may close this issue.

3 participants