micromatch
remove dependency on path
fixes https://github.com/micromatch/micromatch/issues/175
This removes all dependency on the path module from nodejs. This makes it easier for users downstream to bundle micromatch for use in browsers when using pre-configured bundlers like when using the Angular CLI or create react app
The tests are broken now. They need to be fixed. Check for path.sep was there because we have been changing it in tests, I think.
Ah right, just saw the comment here: https://github.com/micromatch/picomatch/blob/e7865c82ffe5e96555bdff57e855a37210a733fe/test/support/index.js#L5-L7
We could require path back in, making it a dynamic require but i'd like to avoid that if possible. Do you still think this comment is valid? While I agree, it's possible that the user can mutate path.sep, I don't think this should be a supported use case.
I’m fine with it.
Sent from my iPhone
On Aug 21, 2019, at 6:50 PM, Paul Miller [email protected] wrote:
Let's hear @jonschlinkert 's opinion.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.
@jonschlinkert For all the tests that are using windows specific inputs, with this new setup they should only be run on a windows host, right?. It dosen't make sense to test a windows path when you're running on a host that uses posix paths.
As I recall, I implemented the tests that way, and made the code comments in the tests, to remind myself that there was an edge case and reason they needed to be that way. Unfortunately, I don't remember at this point what the edge case was, and I'm not confident enough in my memory to say if I'm even correct. I'm not saying you're wrong, as you very well may be correct.
It dosen't make sense to test a windows path when you're running on a host that uses posix paths.
As my 10-year old nephew always says: "Never? Never ever ever?"
It sounds like you're saying that I'm wrong and that there are no edge cases that your changes will effect, and that you 100% aware of all edge cases on all platforms. Is that correct? Are you positive that there are no scenarios in which a user on a non-windows platform would see windows paths, over a remote connection or otherwise?
I'm not saying it happens, or that it happens often, I'm saying that it seems like there was a scenario where it happened, and I'm not 100% sure that it won't.
Hey and just to be clear, I do appreciate the dialog and contribution. I'm just usually skeptical when an absolute assertion is made about Windows paths.
Im not confidant that I know off the top of my head all use cases. However, lets go back to the core issue here.
We currently do a check to see if path.sep is some value.
By allowing the user and our testing suite to mutate this value they can "simulate" being on a different OS.
However, in the situation where a user needs to process a windows path a\\b\\c on linux, we provide the { windows: true } option as a public API.
I propose that that since we already have a public API for supporting windows on a non windows OS, we don't need to do the path.sep check.
So for our testing suite we can test windows works on linux with { windows: true } and for other tests where this is not true, using a windows path as input, we should only test it on a Windows OS host.
An alternative for our testing suite is to instead mutate process.architecture instead
, but again it's not a supported public API.
I'd also just like to point our that users should never modify path.sep, or process.architecture
They'll run into more issues than just issues here.
@jonschlinkert So whats the plan here.
Are we going to go ahead with this? Knowing that we cannot use path.sep as platform detection anymore?
So whats the plan here.
Sorry for the really late reply. I think it's fine, but I just need to have a day where I can really dedicate some time to reviewing this comprehensively before I merge. Everything you said and did makes a lot of sense, I just need to digest it.
Any progress in here? Our frontend builds is failing now because of path dependency in picomatch.
path and other node internals like os have been removed in #73, so this PR can be closed
If you want to use picomatch without any node.js dependencies, you can use this for now https://www.npmjs.com/package/picomatch-browser
