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

Feature request: pass picomatch to the builder #89

Closed
glenn2223 opened this issue Feb 14, 2023 · 4 comments
Closed

Feature request: pass picomatch to the builder #89

glenn2223 opened this issue Feb 14, 2023 · 4 comments

Comments

@glenn2223
Copy link

It would be nice to be able to pass picomatch directly to fdir for better bundling/terser support.

Though I did notice you kind of mention it here, I just wanted to bring it up as a specific use-case feature request.


If I find time and you're open to the idea (to save wasted time 😂), I may be able to submit a PR.

I was thinking that this would have to be a new entry point to make the most sense, though caching becomes an issue. This method would allow for the best portability/adaptability and lean (sort of) towards the pluggability aspect. It would be something like:

globWithMatcher(matcher: Matcher) {
  this.options.filters.push((path) => matcher(path));
  return this;
}

interface Matcher {
  (test: string): boolean;
}

// call it like
new fdir({
    includeBasePath: true,
    resolvePaths: true,
    suppressErrors: true,
    // any other options here
})
    .globWithMatcher(
        picomatch(
            [
                "**/*.scss",
                // Any others
            ],
            {
                ignore: [ 
                    // Stuff
                ],
                dot: true,
                nocase: true,
            }
        )
    )
    .crawl(crawlPath)
    .withPromise();
    

If you'd rather have caching then you could extend the globWithoptions (though it's quite counterintuitive) or change the above method:

type globMatcher<T> = (
  glob: string | string[], 
  options: T
) => Matcher;

interface Matcher {
  (test: string): boolean;
}

// THEN globWithOptions becomes
globWithOptions(
  patterns: string[], 
  options: PicomatchOptions,
  matcher?: globMatcher // removing typing if this is the option
): this;

// OR globWithMatcher becomes
globWithMatcher<T>(
  patterns: string[], 
  options: T,
  matcher: globMatcher
): this;
@thecodrr
Copy link
Owner

@glenn2223 I am not sure I see the benefit of exposing such an API. What's the difference between this and globWithOptions API?

@glenn2223
Copy link
Author

glenn2223 commented Feb 15, 2023

There are a few reasons to have this new API:

  • broaden the glob compatibility
    • moving away from being strictly picomatch (see examples)
    • adding support for micromatch, minimatch, etc
    • users can even create custom functions (by simply implementing a (string) => boolean method)
  • when tersering/bundling, rather than having a reference that you need to successfully require("picomatch) on; you can just pass it directly in a call (this is where the difference/issue with globWithOptions comes in)

Examples (based on name globUsing - not passing any options, for brevity)

new fdir()

// picomatch
  .globUsing(picomatch('*.foo'))

// minimatch
  .globUsing(new minimatch('*.foo').match)

// micromatch
  .globUsing(micromatch.matcher('*.foo'))

// nanomatch
  .globUsing(nanomatch.matcher('*.foo'))

// CUSTOM (just `endsWith`)
  .globUsing(
    (input: string) => 
      input.endsWith('.foo') ||
      input.endsWith('.bar')
  )

// CUSTOM (function where other stuff can happen)
  .globUsing(cM.matcher);
/* WHERE */ const cM = customMatcher('*.foo');

function customMatcher(pattern: string)
{
    // Fake picomatch
    const pM = ((pat) => (input: string) => input.endsWith(pat))(pattern),
    checkedList: string[] = [],
    matchList: string[] = [];

   return { matcher, checkedList, matchList };

   function matcher(input: string) {
        checkedList.push(input);

        const result = pM(input);

        if (result) {
            matchList.push(input);
        }

        return result;
    }
}

// All this is based on:
export declare class Builder<TReturnType extends Output = PathsOutput> {
    globUsing(matcher: Matcher): this;
}

export interface Matcher {
  (test: string): boolean;
}

Now, I know these can be implemented using the filter call. But, it's not really clear that this can be a glob approach (personally speaking)

@louisscruz
Copy link

I'm landing at this issue due to bundling issues related to the dynamic requiring of picomatch. Is there a chance @glenn2223 's proposal could be considered? Otherwise, I'm not quite sure the best way to ensure the dependency is available in the bundle through a require call without doing some pretty unfortunate stuff with my bundling tools.

@SuperchupuDev
Copy link
Contributor

i believe #98 implemented this, but the old picomatch functionality was left as a default so the change is not breaking

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

No branches or pull requests

4 participants