-
Notifications
You must be signed in to change notification settings - Fork 62
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: allow custom glob functions other than picomatch #98
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want an imperative API (withGlobber
or something similar) for this option as well!
happy to update so we have a setter too if that works! we can have will wait and see what @thecodrr thinks 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look mostly fine (haven't looked in depth yet).
We'll also need a withGlobFunction
method to keep this consistent.
should be sorted now 👍 i loosened the types on the way in to reason for this is because |
@thecodrr any chance you can take another look at this? |
@43081j super busy right now but will take a look tomorrow! |
all good. let me know if you need any help same with the other roadmap stuff you wanted to get done, happy to help |
the documentation.md file should be updated with the new options i think? looks like you missed that |
This allows you to pass in your own `globFunction` rather than picomatch, so we're not forcing a choice here (someone may want to use a simpler library or a newer picomatch). Example: ```ts function globFunction( patterns: string | string[], ) { return (test) => { // some glob match logic }; } const api = new fdir({globFunction}) .glob('*.js') .crawl(cwd); const files = await api.withPromise(); ```
I think before this can be merged, we should test the API with some other glob matching libraries and see how compatible & easy this is to use. The |
That's true, I'll try it out when I get chance The types do infer the options parameter but maybe we don't need to do that, and can just rely on people binding it in (instead of having globWithOptions) |
@thecodrr I tried this with zeptomatch: const globFunction = (glob: string) => (path: string): boolean => zeptomatch(glob, path);
const api = new fdir({globFunction})
.glob("**/*.js")
.crawl("node_modules");
const files = await api.withPromise(); the glob function looks funky here just because zeptomatch doesn't return a factory function, but that's all good. it seems to work fine 👍 |
@thecodrr any chance I can get some time from you to move this along? 🙏 |
This allows you to pass in your own
globFunction
rather than picomatch, so we're not forcing a choice here (someone may want to use a simpler library or a newer picomatch).Example:
benchmark seems to be about the same as before on my machine
this also means the options stay strongly typed for whatever glob function you use i think. some example of that: