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

Change glob matcher to picomatch #316

Merged
merged 2 commits into from
Aug 19, 2022
Merged

Conversation

cometkim
Copy link
Contributor

Fixes #244

@mrbbot
Copy link
Contributor

mrbbot commented Aug 17, 2022

Hey! 👋 Thanks for the PR, and apologies I'm only just getting round to it.

I've added a few more test cases, and made some changes as a result.

Firstly, instead of passing all globs to picomatch(), I'm only passing those that don't start with a !. Without it, this test was failing:

t.false(matcher.test("src/image.jpg"));

Secondly, I've enabled the contains option. I'm hesitant to do this, as it means a test like...

t.false(matcher.test("/one/two.txt/three.bad"));

...passes, but without it, this test was failing:

t.true(matcher.test(path.join(process.cwd(), "src/index.js")));

As we resolve to absolute paths in the ModuleLinker, this needs to work. Can you think of any other ways of making this work? If not, I'll merge this as is, since I think in this case, it's better to match more paths than less. You can always add exclusions with !....

@mrbbot
Copy link
Contributor

mrbbot commented Aug 19, 2022

I'm planning to get a release out today and would like to include this, so I'm going to merge it now. 👍

@mrbbot mrbbot merged commit e99db4d into cloudflare:master Aug 19, 2022
mrbbot pushed a commit that referenced this pull request Sep 7, 2022
* Change glob matcher to picomatch

Fixes #244

* Add more `globsToMatcher` test cases and enable `contains` option

Co-authored-by: MrBBot <me@mrbbot.dev>

(cherry picked from commit e99db4d)
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.

Workers using local modules cannot be mounted
2 participants