Skip to content
This repository has been archived by the owner on Jul 29, 2022. It is now read-only.

Multiple Globs conflict when using both root-relative and file-relative globs #4

Closed
brillout opened this issue Mar 29, 2022 · 8 comments

Comments

@brillout
Copy link
Contributor

For example:

const modules = import.meta.importGlob([
  './*.js',
  '/*.js',
])

This requires two different fast-glob runs because ./*.js has cwd: basename(id) while /*.js has cwd: config.root.

We can solve this by discriminating globs into two buckets (one bucket per cwd value). Then we make one fast-glob run per bucket.

But things can get complex if we want to add support for parent-globbing import.meta.importGlob('../../*.js').

I'm wondering if Multiple Globs are worth the added complexity? Are Multiple Globs only about increasing performance by avoiding multiple fast-glob runs? Or is there another use case I'm not seeing?

Proposal: we drop support for Multiple Globs. But maybe I'm missing something here.

Alternatively, a solution would be:

// cwd: config.root
import.meta.glob(['*.js', '*.ts'], { from: 'root' })
// cwd: basename(id)
import.meta.glob(['*.js', '*.ts'], { from: '.' })
// cwd: `${basename(id)}/../../..}`
import.meta.glob(['*.js', '*.ts'], { from: '../../..' })
@antfu
Copy link
Owner

antfu commented Mar 29, 2022

We could normalize the glob before passing to fast-glob

Multiple Globs are worth the added complexity. Drop support for Multiple Globs

There is not much complexity to add since fast-glob and micromatch support multiple globs out-of-box. Or rather I think this is how glob supported to work. Like you might need to ability to ignore some files with the pattern, and sometimes one ignore rule is not good enough for the need (meaning you are shipping extra bits to production). Having a unified place for including/excluding globs feels more natural to me. Since you need two patterns for including and excluding, why not just allow multiple ones to fix more needs? Otherwise, it could cause more complexity on the userland IMO.

@antfu
Copy link
Owner

antfu commented Mar 29, 2022

I will add more tests to cover the usage. I guess from might be a bit too much and ../ feel more like how you would do in static imports.

@antfu antfu closed this as completed in e2fca67 Mar 29, 2022
@brillout
Copy link
Contributor Author

I see, makes sense.

But I think we still need to run fast-glob once per cwd.

E.g. your last commit makes fast-glob significantly slower. At https://github.com/brillout/vite-plugin-ssr/tree/0.4.0/examples/react before the commit the glob result was instant and now it takes around 10 seconds.

@brillout
Copy link
Contributor Author

Ah, I think it's because of node_modules. Let me check.

@brillout
Copy link
Contributor Author

Yes, it's because it crawls into node_modules.

E.g. your last commit makes fast-glob significantly slower. At https://github.com/brillout/vite-plugin-ssr/tree/0.4.0/examples/react before the commit the glob result was instant and now it takes around 10 seconds.

I was wrong, it's not your commit. It's because I removed the node_modules ignore option from my test.

@brillout
Copy link
Contributor Author

AFAICT it works out with the latest changes.

I see only two problems.

  1. Since cwd is config.root (see my PR), fast-glob will always search through the entire user's project. Even if the glob is set to search only one directory with only a few files. But I guess that's ok.

  2. The user cannot glob outside of config.root, which I think is also ok.

In the future, we can use the common ancestor directory of all glob roots as cwd. Although the most performant way would be to run fast-glob once per branch. That would solve both 1. and 2.

The best would be to write a patch for fast-glob so that fast-glob is clever enough to automatically reduce its search space. So that we don't have to set cwd anymore. But I'm not sure how responsive the fast-glob maintainer is and how fast the PR would get merged.

@antfu
Copy link
Owner

antfu commented Mar 29, 2022

I guess the common ancestor directory is doable for us and makes sense. Or maybe you can do a very-fast-glob package that solve this automatically :P

@brillout
Copy link
Contributor Author

Although the most performant way would be to run fast-glob once per branch. That would solve both 1. and 2.

I further dug into that but it turns out to be not that easy to implement.

The best would be to write a patch for fast-glob so that fast-glob is clever enough to automatically reduce its search space.

Also had a quick look into that, but couldn't find a straightforward way to patch fast-glob.

I think the current version is performant enough. But I created a new ticket for it in case fast-glob becomes the bottle neck for some users #8.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants