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

Glob import does not refresh in SSR - minimatch vs micromatch #5511

Closed
7 tasks done
frandiox opened this issue Nov 2, 2021 · 8 comments · Fixed by #5610
Closed
7 tasks done

Glob import does not refresh in SSR - minimatch vs micromatch #5511

frandiox opened this issue Nov 2, 2021 · 8 comments · Fixed by #5610

Comments

@frandiox
Copy link
Contributor

frandiox commented Nov 2, 2021

Describe the bug

The observed bug is that import.meta.glob('...') does not detect newly added files under some circumstances.
I've only seen this during SSR and using a pattern like ..../*.(js|ts). It works when using ..../*.[jt]s) instead.

I think the reason is that import.meta.glob uses fast-glob to match files, which internally uses micromatch.
However, the watcher uses minimatch instead to track new files. Apparently, the syntax of those two is different.

See this example:

const minimatch = require("minimatch")
const micromatch = require("micromatch").isMatch

const newFile = 'pages/name.server.jsx'
const pattern = 'pages/**/*.server.(jsx|tsx)'

console.log(
  minimatch(newFile, pattern), // false
  micromatch(newFile, pattern) // true
)

Related issue: #2734

Reproduction

https://stackblitz.com/edit/vitejs-vite-5mh3tc?file=entry-server.js

  1. Run npm run dev
  2. Add new JS or TS files to files directory. The view won't be refreshed automatically.
  3. Toggle the glob comments in entry-server.js.
  4. Add files again. The view should now refresh.

System Info

System:
    OS: macOS 12.0.1
    CPU: (12) x64 Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
    Memory: 3.98 GB / 32.00 GB
    Shell: 5.8 - /bin/zsh
  Binaries:
    Node: 16.5.0 - /opt/dev/sh/nvm/versions/node/v16.5.0/bin/node
    Yarn: 1.22.17 - /usr/local/bin/yarn
    npm: 7.19.1 - /opt/dev/sh/nvm/versions/node/v16.5.0/bin/npm
    Watchman: 2021.10.18.00 - /usr/local/bin/watchman
  Browsers:
    Chrome: 95.0.4638.54
    Safari: 15.1
  npmPackages:
    vite: ^2.6.0 => 2.6.13

Used Package Manager

npm

Logs

No response

Validations

@frandiox
Copy link
Contributor Author

frandiox commented Nov 2, 2021

@patak-js I can make a PR to fix this but I'm not sure what's preferable:

  1. Replace minimatch with micromatch everywhere => Smallish breaking change
  2. Replace fast-glob with something that uses minimatch internally => Breaking change
  3. Replaceminimatch with micromatch only in the place where it checks the globs => No breaking change, but requires bundling 2 glob libraries and can be confusing.

@patak-dev
Copy link
Member

It is incredible that we have this issue, and I'll add one more to the table. @rollup/pluginutils createFilter is using picomatch... that is very similar to minimatch. In the Rollup docs, it talks about minimatch here, but it is pointing to pluginutils (that uses micromatch). I think they are basically the same though. There is a comparison table here

Given that Vite should align itself with the rollup ecosystem, I think we should migrate to use picomatch everywhere, so I vote to implement 2 here. If you send the PR, we can discuss it this Friday with the team.

@benmccann
Copy link
Collaborator

We ran into this as well in SvelteKit. Issue about it here: sveltejs/kit#2374. We came up with another solution, which was to use functions rather than globs: sveltejs/kit#2430

@patak-dev
Copy link
Member

Thanks for the pointer @benmccann, I like the idea of providing functions as the main way to achieve this. I still think we should align with rollup pluginutils, maybe we could offer both ways. I don't know if we are going to be able to remove support for picomatch/minimatch when the rollup ecosystem (and now the Vite ecosystem) is so used to these patterns.

@frandiox
Copy link
Contributor Author

frandiox commented Nov 4, 2021

@patak-js Oh I see. Looks like micromatch depends on picomatch for the basic globs, and then adds some extra stuff to it. I guess the main problem we have then is minimatch.

I think we should migrate to use picomatch everywhere

In that case, do you know any good replacement for fast-glob that only uses picomatch internally? 🤔

@patak-dev
Copy link
Member

fast-glob uses micromatch, and micromatch uses picomatch internally, no? So this lib should be fine, looks like micromatch is a superset of picomatch IIUC.

@frandiox
Copy link
Contributor Author

frandiox commented Nov 5, 2021

@patak-js perhaps I'm misunderstanding something but, in that case, we should not use picomatch directly because it might have inconsistencies with fast-glob (with advanced syntax like braces, I think). Therefore, we go with option 1 that I mentioned earlier, right?

@patak-dev
Copy link
Member

Yes, you are right. Maybe 1, but document that we only support the picomatch subset?

@github-actions github-actions bot locked and limited conversation to collaborators Nov 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants