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

Add new watches API #1763

Merged
merged 5 commits into from
Feb 1, 2024
Merged

Add new watches API #1763

merged 5 commits into from
Feb 1, 2024

Conversation

ef4
Copy link
Contributor

@ef4 ef4 commented Jan 18, 2024

This is just a toy example to motivate a watch api for virtual content. Edit: this has been trimmed down to only include the API we want

Along the way we discovered that vite incorectly implements rollups addWatchFile API because (1) it's supposed to accept directories and doesn't and (2) it's supposed to resolve relative paths relative to the current working directory, not the current request.

This example also needs #1762

@ef4 ef4 marked this pull request as draft January 18, 2024 14:54
let files = readdirSync(dir);
return {
src: `export default [
${files.map(f => `"${f}"`).join(',')}
Copy link
Contributor

@patricklx patricklx Jan 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this specific case could probably also be solved by using vite import.meta.glob('full/path/dir/*') . would it sill not do auto reload in that case?
other cases could solve it by creating imports to files, vite also supports ?raw which would allow to include any file that needs to be in a virtual file.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or you could do both.

import.meta.glob('full/path/dir/*')
export default [
      ${files.map(f => `"${f}"`).join(',')}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, a lot of cases can be done with import.meta.glob, and this was just a toy example to motivate virtual files with dependencies. We should use import.meta.glob where possible.

I don't think our existing entrypoint logic is a good fit for import.meta.glob though, because it needs to account for app-js merging. If you try to do that with multiple globs you will include and evaluate shadowed modules that should not be there, and I don't think a single glob is expressive enough.

@@ -29,7 +50,10 @@ export function resolver(): Plugin {
},
load(id) {
if (id.startsWith(virtualPrefix)) {
return virtualContent(id.slice(virtualPrefix.length), resolverLoader.resolver);
let { src, watches } = virtualContent(id.slice(virtualPrefix.length), resolverLoader.resolver);
virtualDeps.set(id, watches);
Copy link
Contributor

@patricklx patricklx Jan 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefix here the src with import.meta.glob(...watches). and filter watches for the ones containing globs.
files should just work with addWatchFile.
like that you do not need the configureServer code.

let { src, watches } = virtualContent(id.slice(virtualPrefix.length), resolverLoader.resolver);
for (w of watches) {
  if (w.includes('*')) {
   src =  `import.meta.glob($'{w}')\n` + src;
  } else {
   this.addWatchFile(w);
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our actual use cases need more sophisticated filtering than we can do with import.meta.glob. For example, we need a way to include "node_modules/some-addon/app/components/foo.js" but only if "./components/foo.js" doesn't exist. If we do that with globs you will always evaluate the addon copy, even when it should not evaluate.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mport.meta.glob is not eager, but can be configured to be so with params, so it shouldn't evaluate. but not sure if thats enough for the use case you are looking for

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, in the case of entrypoints we need the eager: true variant.

@mansona mansona changed the title [wip] sketching watch API Add new watches API Feb 1, 2024
@mansona mansona added breaking enhancement New feature or request and removed breaking labels Feb 1, 2024
@mansona mansona marked this pull request as ready for review February 1, 2024 13:00
ef4 and others added 5 commits February 1, 2024 13:39
Edit: This started as a sketch and evolved to how we want it to look

Along the way we discovered that vite incorectly implements rollups `addWatchFile` API because (1) it's supposed to accept directories and doesn't and (2) it's supposed to resolve relative paths relative to the current working directory, not the current request.
Copy link
Member

@mansona mansona left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 🎉

@mansona mansona merged commit f361b71 into main Feb 1, 2024
93 checks passed
@mansona mansona deleted the watch-api branch February 1, 2024 17:32
@github-actions github-actions bot mentioned this pull request Jan 31, 2024
@github-actions github-actions bot mentioned this pull request Apr 4, 2024
@github-actions github-actions bot mentioned this pull request Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants