-
-
Notifications
You must be signed in to change notification settings - Fork 1
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
Reduced test case demonstrating incompatibility with Node "path" module #1
Conversation
This explains both issues #2093 & #2094. The reason it was working in your reduced test case and it wasn't working in my environment was because my environment was using Nodes "path" module to generate the strings where as the reduced test case was only using explicit strings. Every file added in this commit is a demonstration of the bug in action. They were generated by gulp when the should not be.
'src/_scripts/**/*', | ||
'!src/_scripts/{**/\_*,**/\_*/**}', | ||
path.join('src','_scripts','/**/*'), | ||
'!'+path.join('src','_scripts','/{**/\_*,**/\_*/**}'), | ||
]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should be using {cwd: src}
and not using path.join()
on glob patterns. Glob patterns are regular expressions, not paths. fwiw, using the path
module might lead to unexpected patterns that do not work. For example, on windows path.join()
will use backslashes. In regular expressions, backslashes are not path separators, they are escape characters.
The point is that both Gulp 3 and Gulp 4-alpha.2 supported this feature so people will expect to be able to use that syntax in Gulp 4.0.0, especially since path is a native Node plugin. In my experience on Windows, path.join() is essentially the equivalent of writing ['folder ','path'].join('\'). It does feature back slashes but they are escaped backslashes so I don't see that being an issue. Not supporting path.join() could break a lot of gulp set ups and it isn't obvious why Gulp isn't acting the way it's supposed to. People just think Gulp is broken and might try and swap to using something else. Can there at least be an error that is thrown when Gulp 4 encounters path.join() if you don't want to make it compatible? Or maybe just have it documented in the Gulp documentation for src, dest, & watch that path.join() should not be used to merge folder paths and instead use the It's really hard to figure out why Gulp isn't working properly if you do happen to use path.join() in your gulp file, especially since it has worked just fine with every previous version of Gulp. |
There's no way to tell that Below I'm linking to some related issues about why there are breaking changes in Gulp 4 and why
Any help with updating documentation is appreciated. Check with the last bullet in the list to see what progress has been made. |
Joining a glob with node's path module is like creating a regex with the path module. As @doowb mentions, there is no way to know when you intended to use |
I figured out what the real bug in Gulp 4 was :) This explains both issues #2093 & #2094.
The reason it was working in your reduced test case and it wasn't working in my environment was because my environment was using Nodes "path" module to generate the strings where as the reduced test case was only using explicit strings.
Every file added in this commit is a demonstration of the bug in action. They were generated by gulp when the should not be.