`path` and `process` dependencies

[silverwind]

I'd like to run this module in a browser. Would it be resonable to ask these dependencies to be made optional?

[silverwind]

It appears this module works in browsers via the magic of webpack's node shims. But there are a few minor issues:

1. process.version used here is '' in webpack so this check will never be successful. Feature detection would be preferred instead.

2. Only basename and sep is used from path. I suggest doing const {basename, sep} = require('path') instead of importing the whole module to enable tree shaking. Thought I do guess that a smart enough bundler might be able to treeshake it regardless, so the point may be moot.

[jasonwilliams]

Sorry for braindump.....

@silverwind using node shims is no longer the recommended direction from Webpack v5 onwards. Instead, this package should generate 2 bundles:

• A "node" version which is just a bundled version of what's there already.
• A "browser" version which doesn't try to use processs and path.

see https://stackoverflow.com/questions/26063480/how-to-simultaneously-create-both-web-and-node-versions-of-a-bundle-with-web as an example. Actually target can take an array of options so you could just do 1 config with target set to ['web', 'node']

Once that's done you can add"browser": "./path/to/browser-version.js" to the package.json.

As for process i think you can just check if process exists in the code before using it, then fall back to something reasonable. With path you can have some polyfill object in the webpack config which is used as a fallback. sep can always return / and basename can be added too. You can do this by creating an alias in webpack and pointing it to a ./path-polyfill-file.js https://webpack.js.org/configuration/resolve/#resolvealias

If you decide to go for the single-config option (explained above) then use fallback instead of alias. This means when Webpack generates the "web" version it will use the polyfill file instead of the native path https://webpack.js.org/configuration/resolve/#resolvefallback works too (and may even be better)

This should be a relatively easy PR for anyone to do.

[silverwind]

~~I think the best course of action would be to extract just the matcher (basically .isMatch) to its own package. The matcher itself will never have dependency on the file system so should be universally portable.~~

Edit: I forgot, this package is advertized as just a matcher, so I think those node dependencies need to go in any case to make it usable in deno and the browser.

[jasonwilliams]

@silverwind is this a change you're planning to PR?

[silverwind]

Not planning on a PR, maybe a fork/rewrite would be more appropriate. I think with sep made an option (or dropped entirely because nobody cares about Windows paths), the module's bundle size could be reduced by a lot. Just looking at the API, it seems to have a few redundant methods that are ripe to remove.

Edit: Module is 20kB minfied which certainly seems too much for what it does.

[jasonwilliams]

Ok in that case i think https://github.com/micromatch/picomatch/issues/64#issuecomment-732228132 is still worth doing, as a rewrite is a quite a big change and maybe out of scope for the issue we have here.

also TIL about bundlephobia

In terms of size, i don't see any bundling or building happening in this project. So adding webpack would also help with that (treeshaking etc)

[jasonwilliams]

What do @mrmlnc or @jonschlinkert think about this?

[silverwind]

So I took a look at removing those dependencies, path.basename is easily replaced by a regex replacement but process is being used to alter the module's behaviour when running on Windows (on Windows, this module seems to accept both forward and backward slashes as path separators, on other platforms only forward slashes).

So to cleanly remove process, I think a windows option needs to be added and has to be passed in by the user like process.platform == "win32" which of course is a breaking change.

[silverwind]

https://github.com/micromatch/picomatch/pull/73