Skip to content
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

Merged
merged 3 commits into from
Sep 26, 2024

Conversation

43081j
Copy link
Contributor

@43081j 43081j commented Jun 24, 2024

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:

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();

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:

interface GlobOptions {
  x: number;
}

declare function globFunction(patterns: string|string[], options?: GlobOptions): GlobMatcher;

const api = new fdir({globFunction});

api.globWithOptions(
  ['*.js'],
  {
    x: 100, // works
    y: 200, // errors, since `GlobOptions` has no such prop
  }
);

Copy link

@SukkaW SukkaW left a 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!

@43081j
Copy link
Contributor Author

43081j commented Jun 29, 2024

happy to update so we have a setter too if that works!

we can have withGlobFunction or some such thing which sets the same property

will wait and see what @thecodrr thinks 👍

Copy link
Owner

@thecodrr thecodrr left a 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.

@43081j
Copy link
Contributor Author

43081j commented Jul 2, 2024

should be sorted now 👍

i loosened the types on the way in to unknown and infer the strong types when you call globWithOptions

reason for this is because picomatch exports a conditional type which confuses inference here (we end up with it having a return type of never because the compiler can't infer it)

@43081j
Copy link
Contributor Author

43081j commented Jul 22, 2024

@thecodrr any chance you can take another look at this?

@thecodrr
Copy link
Owner

@43081j super busy right now but will take a look tomorrow!

@43081j
Copy link
Contributor Author

43081j commented Jul 27, 2024

all good. let me know if you need any help

same with the other roadmap stuff you wanted to get done, happy to help

@SuperchupuDev
Copy link
Contributor

the documentation.md file should be updated with the new options i think? looks like you missed that

43081j added 3 commits August 25, 2024 20:55
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();
```
@thecodrr
Copy link
Owner

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 glob & globWithOptions functions were designed to be very picomatch specific so I am not sure if the current design and implementation still makes sense if other libraries are used.

@43081j
Copy link
Contributor Author

43081j commented Aug 27, 2024

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)

@43081j
Copy link
Contributor Author

43081j commented Aug 29, 2024

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

@43081j
Copy link
Contributor Author

43081j commented Sep 24, 2024

@thecodrr any chance I can get some time from you to move this along? 🙏

@thecodrr thecodrr merged commit 2cf7a9d into thecodrr:master Sep 26, 2024
@43081j 43081j deleted the custom-globs branch September 27, 2024 04:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants