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 basic watch file support #2105

Closed
wants to merge 1 commit into from
Closed

Conversation

mansona
Copy link
Member

@mansona mansona commented Sep 11, 2024

I'm not saying that this PR fixes all of our needs for watching app files, but since we are targeting real files on disk now and not a rewritten app this seems like it goes a decent way towards fixing things 🤷 If we're going to encourage people to try this out this week then maybe we can merge this and work on tests for it in our next office hours?

@@ -147,7 +147,7 @@ export function renderEntrypoint(

return {
src: entryTemplate(params),
watches: [],
watches: ['app/**/*'],
Copy link
Member Author

Choose a reason for hiding this comment

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

I've tried this locally and it seems to be all we need to get it working 🤷

Copy link
Contributor

Choose a reason for hiding this comment

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

This is another place where we need to be doing things as in d850898

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of hard-coding the string "app".

@@ -330,7 +330,7 @@ function shouldSplitRoute(routeName: string, splitAtRoutes: (RegExp | string)[]

export function getAppFiles(appRoot: string): Set<string> {
const files: string[] = walkSync(appRoot, {
ignore: ['_babel_filter_.js', 'app.js', 'assets', 'testem.js', 'node_modules'],
directories: false,
Copy link
Member Author

Choose a reason for hiding this comment

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

I removed the ignore because we now target the app folder as our appRoot so we don't need these ignores, and I added directories false because we only care about files 😂

@mansona mansona added the enhancement New feature or request label Sep 11, 2024
@patricklx
Copy link
Contributor

im trying this in my vite watch mode pr, because currently its not detecting any new files. This is also not working, i think its missing changes in vite/resolver.ts

@patricklx
Copy link
Contributor

patricklx commented Sep 13, 2024

i think it should resolve to the full path to /app dir, without *.
right, watches: [resolve('./app')], fixes it. my watch mode tests are passing with that

@patricklx
Copy link
Contributor

maybe it would make sense to make the template only component virtual, similar to virtual pair component, instead of making it a fake file which can collide with the real one on disk? maybe it would make it easier to make HMR work in vite. and making it virtual would also remove all the metadata code in hbs plugin.

@ef4
Copy link
Contributor

ef4 commented Nov 27, 2024

This got superseded, virtual-entry point now watched the whole app dir.

@ef4 ef4 closed this Nov 27, 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