Unc path

[stefanpenner]

This is a hybrid of @staticshock and my work.

• unit tests that cover the actual WinPath implementation
• integration test – using mock-fs and some path.sep and path.resolve faking, simulate the windows environemnt.
• pending minimatch fix – it doesn't have mock-friendly platform specific code
  • PR prefer feature detection over platform sniffing minimatch#63
  • merge + release
• feedback
• re-confirm the latest refactoring still solves the original issue – it was earlier

---

future work:

• get tests passing on windows
  • make tests at-least run
  • make them all pass
• appveyor

[stefanpenner]

@staticshock & @isaacs I believe this is ready for review. I'll gladly make changes, so keep me posted.

[stefanpenner]

the faking code needs more work actually, although the final code works in windows. The test fails, investigating further. that specific test passes in windows now, but many other unrelated failures exist.

[isaacs]

This is pretty close!

I inadvertently optimized away your minimatch change, but then I fixed it on 2.0.7.

I am still running into a bit of a weird issue with the test, because the path-is-absolute module is rather insistent on using process.platform, and it's also used throughout the codebase a bit. I was able to get this specific test to work, but only because isAbsolute('/baz') is the same on Windows and Posix.

I think the right approach here, though it's more lines changed, is to pass the platform as an option to Glob, and then load all the appropriate separator/absolute/etc stuff onto the glob object, so it never has to look at process.platform or path.sep or whatever.

I'm thinking something like this in the setopts function in common.js:

self.platform = opts.platform || process.platform
self.isAbsolute = isAbsolute[self.platform] || isAbsolute;
self.resolve = path[self.platform].resolve || path.resolve;
self.sep = path[self.platform].sep || path.sep;

And then, throughout glob.js and sync.js, make sure to always call the methods off of this rather than path.<whatever>() or process.platform.

This also means that, if at some point we need to change just one of these fields, or use AOP to debug those function calls, we can either make those fields configurable, or instantiate an object and just change the relevant bits.

Of course, this can lead to glob doing something weird in some cases, but it'd make it so that we can test other windows irregularities in the future more easily, with a lot less module cache tinkering.

What do you think?

[stefanpenner]

What do you think?

I'm on-board with passing platform in. In-fact it was something I considered but opting out of merely to avoid a more controversial PR. So if you are +1, I'll make it so.

Just a heads-up I'll likely have time again tomorrow to work on this. That being said, with more drastic changes I will likely want to get all the windows tests working. So the changes will likely be a-bit more involved. Is that ok?

[isaacs]

I'm totally ok with involved changes that improve testability, windows support, and make the code easier to follow (including fewer hacky workarounds in tests).

Of course, bigger PR size tends to make for more back and forth in the review process, but I think we're pretty aligned on what needs to happen for this.

[stefanpenner]

Of course, bigger PR size tends to make for more back and forth in the review process, but I think we're pretty aligned on what needs to happen for this.

Ya I'm fine with thorough back + forth, as this module had 11,577,444 downloads in the last month quality and compatibility should be of upmost priority.

[stefanpenner]

@isaacs sorry for the delay, been under the weather. Anyways, this is ready for another review.

One question (and the last failing test), is path in node 0.10.x doesn't have path[platform]...

• should I polyfil it ?
• am i missing something more obvious ?

[isaacs]

UGGHHHH that's right.

I'd say just skip that test in node <= 0.10. The code should still work fine, and since this is really just a matter of string juggling stuff, it should be fine to run the tests in only later versions.

[stefanpenner]

@isaacs whats the ideal way to detect node 10? Something like https://www.npmjs.com/package/nversion ? Or a funky regexp

[isaacs]

I'd throw something like this in there:

var skip = false
if (/^v0\.(10|[0-9])\./.test(process.version)) {
  skip = 'Does not work on Node < 0.12'
}

test('some test name', { skip: skip }, function (t) { ...

[isaacs]

If you wanna just leave the tests failing in 0.10, that's fine, too. I can work it out when I merge it in. If it's passing in 0.12 and io.js, then I can take it from there :)

[isaacs]

Ok, just reviewed. The test looks perfect, and nothing in the code jumps out at me as problematic.

If you think this is ready to go, I can merge and release this weekend or early next week.

[stefanpenner]

If you wanna just leave the tests failing in 0.10, that's fine, too. I can work it out when I merge it in. If it's passing in 0.12 and io.js, then I can take it from there :)

I'll add the skip 📦

Ok, just reviewed. The test looks perfect, and nothing in the code jumps out at me as problematic.

👍

If you think this is ready to go, I can merge and release this weekend or early next week.

My use-cases work correct, I tried several ways to break it and couldn't find anything. I am fairly confident. But full disclosure, my windows-foo is weak.

Also, I will be on vacation new week. So if you release I wont be able to deal with fall-out, it will be on you. If you wait a week, I can help out with any fall out.

Your call sir.

[stefanpenner]

sorry, i am unclear why the skip isn't working as expected :(

[stefanpenner]

lets actually hold off, I may have noticed other scenarios... such pandora's box.

[stefanpenner]

rebased, going to fire up the old windows gaming rig after breakfast and see whats next.

[stefanpenner]

Tests should now correctly run, even on old versions of node. Switching to windows now, lets see whats left on this side of things.

[stefanpenner]

@isaacs i have added the appveyor configuration, is it possible for you to set up the appveyor.com side of things? I would doit, but then node-glob would end-up stefanpenner/node-glob on appveyor.

It should be as simple as singing up for the free tier, and adding this repository. As for making the tests pass on windows, I have put together a short triage list, and will work on them this afternoon. If I run into any blockers, I will report back.

[isaacs]

It looks like the issue is that the WinPath stuff in the setopts function in common.js is messing with root and path in a weird way, such that glob(process.cwd()) can't return anything, because it ends up having a root setting of c: and a pattern of /path/to/cwd.

The mocking and testing stuff looks pretty good. I think it's the right approach.

On master right now, all the tests pass on Windows. I'd recommend rebasing, and then applying the changes one by one and making sure that tests pass at each commit.

[isaacs]

No longer an issue in v9.