remove dependency on path #23

Toxicable · 2019-08-11T03:52:00Z

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

Toxicable · 2019-08-11T03:55:41Z

lib/utils.js

@@ -32,7 +36,7 @@ exports.isWindows = options => {
  if (options && typeof options.windows === 'boolean') {
    return options.windows;
  }
-  return win32 === true || path.sep === '\\';
+  return win32 === true;


we don't need to check the path sep, since we already know if we're on windows from the above check.
And path.sep is only ever \ when on windows https://nodejs.org/api/path.html#path_path_sep

And path.sep is only ever \ when on windows

Naively, yes. But this also gave us a super easy way to explicitly set \\ as the path separator for tests.

Sure, but as below, I don't think setting path.sep should be a supported use case, so using that logic isn't correct with that in mind

paulmillr · 2019-08-21T18:40:50Z

lib/picomatch.js

-  return regex.test(path.basename(input));
+
+  // if it ends with a trailing / we filter out the last empty item that split will make
+  const basename = input.split(/[\\/]/).filter(v => v !== '').pop();


Move this regex out of the function to save mem.

paulmillr · 2019-08-21T19:24:01Z

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.

Toxicable · 2019-08-21T20:26:36Z

Ah right, just saw the comment here:

picomatch/test/support/index.js

Lines 5 to 7 in e7865c8

    
           // Don't use "path.sep" here, as it's conceivable that it might have been 
        
           // modified somewhere by the user. Node.js only handles these two path separators 
        
           // with similar logic, and this is only for unit tests, so we should be fine.

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.

paulmillr · 2019-08-21T22:50:55Z

Let's hear @jonschlinkert 's opinion.

jonschlinkert · 2019-08-22T18:10:25Z

I’m fine with it.

…

Sent from my iPhone

On Aug 21, 2019, at 6:50 PM, Paul Miller ***@***.***> 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.

paulmillr · 2019-08-22T18:33:14Z

@Toxicable let’s remove tests then!

jonschlinkert · 2019-08-23T10:18:52Z

test/options.js

-      assert.deepEqual(match(['E:\\a\\b\\c.md'], 'E:/**/*.md'), ['E:/a/b/c.md']);
-      assert.deepEqual(match(['E:\\a\\b\\c.md'], 'E:/**/*.md', { windows: false }), []);
-    });
-  });


why were these removed?

jonschlinkert · 2019-08-23T10:19:48Z

test/special-characters.js

+      assert(isMatch('c:\\', '**', { windows: true }));
+      assert(isMatch('C:\\Users\\', '**', { windows: true }));
+      assert(isMatch('C:cwd/another', '**', { windows: true }));
+      assert(isMatch('C:cwd\\another', '**', { windows: true }));
    });


These are wrong. The entire point was to test these using platform detection, and without windows: true

jonschlinkert · 2019-08-23T10:19:54Z

test/special-characters.js

-      path.sep = '/';
+      assert(isMatch('c:\\', '*{,/}', { windows: true }));
+      assert(!isMatch('C:\\Users\\', '*', { windows: true }));
+      assert(!isMatch('C:cwd\\another', '*', { windows: true }));
    });


I don't follow, this test is simulating being on windows right?
Why not only run it on windows then?

jonschlinkert · 2019-08-23T10:20:00Z

test/special-characters.js

+      assert(isMatch('\\\\unc\\admin$', '//*/*$', { windows: true }));
+      assert(isMatch('\\\\unc\\admin$\\system32', '//*/*$/*32', { windows: true }));
+      assert(isMatch('\\\\unc\\share\\foo', '//u*/s*/f*', { windows: true }));
+      assert(isMatch('foo\\bar\\baz', 'f*/*/*', { windows: true }));
    });


Toxicable · 2019-08-23T11:21:08Z

@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.

jonschlinkert · 2019-08-23T13:49:05Z

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.

jonschlinkert · 2019-08-23T13:57:10Z

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.

Toxicable · 2019-08-23T21:07:33Z

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.

Toxicable · 2019-08-30T05:03:19Z

@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?

jonschlinkert · 2019-09-18T19:12:10Z

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.

jonschlinkert · 2021-09-25T21:02:27Z

@dawgwe1 you've been blocked

idenisovs · 2022-01-31T19:13:21Z

Any progress in here? Our frontend builds is failing now because of path dependency in picomatch.

acao · 2022-02-14T11:53:05Z

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

Toxicable commented Aug 11, 2019

View reviewed changes

paulmillr requested changes Aug 21, 2019

View reviewed changes

Toxicable force-pushed the master branch from b95a67a to 7fbd050 Compare August 23, 2019 07:09

remove dependency on path

a08ca13

Toxicable force-pushed the master branch from 7fbd050 to a08ca13 Compare August 23, 2019 07:13

Fabian Wiles added 2 commits August 23, 2019 20:57

test

6714a14

remove windows more

0efa430

jonschlinkert reviewed Aug 23, 2019

View reviewed changes

mroe windows failures

1a69e11

micromatch deleted a comment Sep 25, 2021

Toxicable closed this Mar 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove dependency on path #23

remove dependency on path #23

Toxicable commented Aug 11, 2019

Toxicable Aug 11, 2019

jonschlinkert Aug 23, 2019

Toxicable Aug 23, 2019

paulmillr Aug 21, 2019

paulmillr commented Aug 21, 2019

Toxicable commented Aug 21, 2019

paulmillr commented Aug 21, 2019

jonschlinkert commented Aug 22, 2019 via email

paulmillr commented Aug 22, 2019

jonschlinkert Aug 23, 2019

jonschlinkert Aug 23, 2019

jonschlinkert Aug 23, 2019

Toxicable Aug 23, 2019

jonschlinkert Aug 23, 2019

Toxicable commented Aug 23, 2019

jonschlinkert commented Aug 23, 2019

jonschlinkert commented Aug 23, 2019

Toxicable commented Aug 23, 2019

Toxicable commented Aug 30, 2019

jonschlinkert commented Sep 18, 2019

jonschlinkert commented Sep 25, 2021

idenisovs commented Jan 31, 2022 •

edited

acao commented Feb 14, 2022 •

edited

remove dependency on path #23

remove dependency on path #23

Conversation

Toxicable commented Aug 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulmillr commented Aug 21, 2019

Toxicable commented Aug 21, 2019

paulmillr commented Aug 21, 2019

jonschlinkert commented Aug 22, 2019 via email

paulmillr commented Aug 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Toxicable commented Aug 23, 2019

jonschlinkert commented Aug 23, 2019

jonschlinkert commented Aug 23, 2019

Toxicable commented Aug 23, 2019

Toxicable commented Aug 30, 2019

jonschlinkert commented Sep 18, 2019

jonschlinkert commented Sep 25, 2021

idenisovs commented Jan 31, 2022 • edited

acao commented Feb 14, 2022 • edited

idenisovs commented Jan 31, 2022 •

edited

acao commented Feb 14, 2022 •

edited