Update is-glob dep to ^3.0.0

[es128]

Resolves #533

[es128]

Looks like this is causing some failures in glob-related tests on Appveyor (Windows) consistently that were not there before. These will have to be investigated and resolved before this is merged.

[coveralls]

Coverage remained the same at 98.287% when pulling 19add92 on upgrade-is-glob into f17186d on master.

[es128]

FYI @jonschlinkert

[jonschlinkert]

looking now

edit: I'll have to pull this down to see what's causing the issues. it's not obvious from appveyor what's happening.

[SpaceK33z]

Friendly ping since many people have issues with this :).

[jonschlinkert]

many people have issues with this :).

what specifically are many people having issues with? Nothing has been merged, and afaik no actual issues were ever created for the prior version of is-glob, only speculation.

(edited, I said extglob but meant is-glob initially)

edit 2: sorry I see you are probably referring to all of the linked issues. I'll review

[es128]

@jonschlinkert it's a long-standing issue in chokidar that when given a path to watch that matches the original is-glob regex it would treat it as a glob instead of as an actual file path and not work properly in some cases. The updated is-glob would solve that for parenthesis characters at least, but other things like * would still need a further fix within chokidar.

I have not yet been able to take any time to try to figure out what's going on with these tests on windows.

[jonschlinkert]

I have not yet been able to take any time to try to figure out what's going on with these tests on windows.

Ok, thanks for the summary.

Looks like this is causing some failures in glob-related tests on Appveyor (Windows) consistently that were not there before

Regarding this, I'm wondering if the tests are actually "correct" now. fwiw I'm getting a windows box this week to make testing this stuff easier.

[es128]

The three tests have nothing to do with parentheses, and it's not at all obvious why they started failing. But they failed in both polling and non-polling modes with each node version, so it seems pretty clear that something happened that merits investigation.

It's not necessarily that is-glob has a bug or is doing anything wrong, just need to figure out what's up. The fact that it presents only on windows does make it more challenging to work on.

https://github.com/paulmillr/chokidar/blob/master/test.js#L534
https://github.com/paulmillr/chokidar/blob/master/test.js#L686
https://github.com/paulmillr/chokidar/blob/master/test.js#L1352

[jonschlinkert]

But they failed in both polling and non-polling modes with each node version, so it seems pretty clear that something happened that merits investigation.

Hmm, yeah that is strange. @doowb and I are both working on this right now. Sorry I was on vacation part of last week - there is no easy way to let everyone know that on github.

[jonschlinkert]

edit: I should clarify that (I believe) this is mostly a problem with how test fixtures are defined. I'm not sure if this carries over into the code anywhere.

Ok, @doowb and I have narrowed down the main cause of the "windows" issues. It's not technically related to is-glob, but it's manifesting now since previous versions of is-glob did not respect escaping.

The gist of it is that paths with \\ are being joined to glob patterns, which might result in a glob pattern with something like \\*. This would be (correctly) seen as an escaped character, so is-glob returns false. In previous versions of extglob, it would have been true.

Backslashes are invalid path separators in glob patterns (in all of the globbing libs, node-glob, minimatch, micromatch, etc), so we're getting the correct, expected result from is-glob. If we somehow change the patterns passed by the user, we need to make sure that our paths don't have backslashes, or that at least the very last slash between the new path and the glob pattern is not a backslash. Otherwise, we're just potentially escaping a valid glob character.

Does this make sense?

fwiw normally users aren't slapped on the wrist for defining patterns with \\, because it's impossible to prevent people from doing it. But when the user is passing the pattern directly to libraries like node-glob or micromatch, \\ is just "unixified" to /, and then when something breaks it's easier to explain to the user "don't use backslashes", and even if they do (and/or the user has escaping issues), it's easier to explain what's happening.

[es128]

@jonschlinkert @doowb thank you for uncovering the likely cause. It makes sense.

However, I don't think resolution is totally straightforward. The tests don't specify any backslashes, they simply use path.join(). I would expect some users to be doing the same out in the wild with their glob patterns, and even if that technically violates the glob spec I hope we can find a way to support it.

If the string is actually coming through with \\*, then perhaps the escape character detection has a bug? (i.e. the second backslash is not escaping the asterisk, it is being escaped by the first one)

If the string actually does look more like C:\foo\bar\* then this may be tricky to fix.

[es128]

If your suggestion was that chokidar runs a unixify type operation on the strings before testing with is-glob, then what happens if someone actually was trying to escape an asterisk with a backslash?

[jonschlinkert]

even if that technically violates the glob spec I hope we can find a way to support it.

I think we can.

The tests don't specify any backslashes, they simply use path.join()

__dirname uses path.sep. If you log out the paths in the tests you'll see what's happening (@doowb pointed this out first).

I'd like to find a nice solution for everyone. There at least three open issues on node-glob and minimatch related to exactly this problem (escaping and windows backslashes conflicting). I'll try to link to them when I have a chance. I'm going to think about this and come back to it before I make any suggestions.

[davidwr]

I updated is-glob dependecy to 3.1.0 manually on chokidar and the problem still happens.

I'm trying to watch the folder inside "Program Files (x86)".

What I have to do?

[xsq007]

@es128
Is it possible to merge upgrade-is-glob branch to master and publish a new release?
thanks.

[es128]

@xsq007 no. The issue hasn't been resolved yet and it would break chokidar for windows users.

[xsq007]

@es128
could you share more details about the break on Windows? I wanna evaluate the affection to determine whether to update is-glob to 3.0.0 manually.
Thanks.

[lorenzos]

This is probably bypassable now using the new disableGlobbing option added in #598. That said, how this issue will relate to that new option? Will it be closed and never fixed because there's a workaround now (I hope not)?

[es128]

No, not taking disableGlobbing as a substitute. I still do want to achieve compatibility with is-glob@3

[jonschlinkert]

@es128 the only solution I can think of that seems like a decent compromise is to expose an option to let users choose escaping behavior. but I don't know if that's a good way to solve this. It seems like users will (understandably) get confused or create issues on both sides of the solution, no matter how intuitive or whatever options we expose. just thinking out loud.