gulpjs / glob-stream

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

OS independent square brackets [] #113

Closed pwall2222 closed 1 year ago

pwall2222 commented 1 year ago

In #2346 @iliyahanev explained that globs weren't working properly on different platforms, be it win32 and Linux. I took the task at hand and solved it in the best way possible, because it may not adhere to the coding standards of gulpjs I will explain the problem here. If you pass a non escaped bracket to glob-stream in the cwd this will not work as node-glob (picomatch) will do weird stuff with it and not really do what you want to do, but if you do escape it (with backslash) to-absolute-glob will think "unixify" it (aka converting \ to /) so that also breaks. Then there is another problem, windows escapes brackets with the syntax [[]myfilename] but then most other systems use \[myfilename\] for escaping brackets. To solve all of this first before passing cwds to to-absolute-glob I made sure that the cwd was unescaped (because if we don't do that we could escape it twice since we call getBasePath with the escaped cwd) and then escapped it again (and retrofitted only the cwd back to the glob, without touching the expression itself). That fixes our escaping and no escaping problem with to-absolute-glob but then we also need to escape specifically for windows, since we alredy have escaped brackets we can simply change the escaping (or remove it) to match windows requirements.

phated commented 1 year ago

Thanks for looking into this @pwall2222! There's actually been breaking releases in glob-stream that can't be included in gulp 4 and I'll be working on further breaking changes for gulp 5 in the next couple of weeks. I want to figure out a way to make sure these fixes end up in gulp 5 though

phated commented 1 year ago

I just slammed my head up against this when trying to convert to anymatch 🤦

I also maintain is-absolute-glob and the true bug seems to be in that library, so I'm thinking we should make the fixes there. What do you think?

phated commented 1 year ago

@pwall2222 do you have documentation for the [[]stuff] escaping on windows? Does it only apply to square brackets, or other things like parens or curly braces?

pwall2222 commented 1 year ago

I know that for parens is not necesary but I will try to find the documentation for that. Also I didn't know you were a mantainer on is-absolute-glob but I tryed fixing it on the direct gulp dependency because the issue was mentioned on the gulp repository. But it would make a lot of sense to fix it in is-absolute-glob at least for the converting escapped brackets to forward slashes.

pwall2222 commented 1 year ago

@pwall2222 do you have documentation for the [[]stuff] escaping on windows? Does it only apply to square brackets, or other things like parens or curly braces?

So after a lot of searching for specific windows features and a lot of dead ends I have ended just searching the pattern [[]stuff] and I came across this stackoverflow question, and realised that is not that we are escaping the brackets on windows, rather that we are saying to the glob parser that we have a regex character class that only contains [ and since in regex we can have closing brackets without problem the last one doesn't need "escaping". So I think that was just a small hack that could be used for working around the limitation of to-absolute-glob breaking our paths.

phated commented 1 year ago

Excellent! Thanks for digging so deeply into this ❤️

As you might have noticed, I upstreamed the path escape/unescape logic into to-absolute-glob (and expanded it to handle the glob characters that might exist). This also ensures that the root option is handled correctly too.

I've been experimenting in my own branch and it looks like we can now upgrade glob (which finally supports escaping correctly) and to-absolute-glob (which escapes properly) and then we don't need the workarounds in this project.

Since you expertly separated the tests from the workarounds, I'm going to back out those 2 "workaround" commits and update the dependencies in your branch. Then we can land your test updates to ensure that my anymatch changes don't break the fixed behavior.

Thank you so much!

phated commented 1 year ago

It seems that I don't have commit access, so I'll open a new PR on this repo.

phated commented 1 year ago

This was succeeded by #117

Massive shoutout and thanks to you @pwall2222 🙇