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

fix(hmr): watch files in workspace root #16473

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sapphi-red
Copy link
Member

@sapphi-red sapphi-red commented Apr 19, 2024

Description

fixes #16399
refs #15712

We need to make these values consistent. Otherwise, the cache could be different from the actual file system.


const root = normalizePath(searchForWorkspaceRoot(config.root))

Alternative fix for this is to revert #15712. I'm not sure if which is safer. Maybe reverting it in a patch and reapplying it in the next minor?

@sapphi-red sapphi-red added feat: hmr p4-important Violate documented behavior or significantly improves performance (priority) labels Apr 19, 2024
Copy link

stackblitz bot commented Apr 19, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@sapphi-red
Copy link
Member Author

Hmm, the test fails 🤔

@sapphi-red sapphi-red marked this pull request as draft April 19, 2024 17:34
Comment on lines 460 to 461
...config.configFileDependencies,
...getEnvFilesForMode(config.mode, config.envDir),
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 found that pnpm test-serve /css/ passes if I comment out these lines. Maybe there's a bug here 🤔 But even with that running the whole test (pnpm test-serve) takes a long time. I guess it's because we have many files under the repository.

Copy link
Member

Choose a reason for hiding this comment

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

I'm worried about the performance impact of watching the whole fs root. What I'm thinking now is that we may need to be more fine-grained. Maybe we could automatically watch parent folders in a shallow way (not deep watch) of every imported file for example.
For now, I think we should revert.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: hmr p4-important Violate documented behavior or significantly improves performance (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Imports outside of the root break on file rename
2 participants