-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
perf(remix-dev/vite): use fs watcher to invalidate manifest instead of request time #8164
perf(remix-dev/vite): use fs watcher to invalidate manifest instead of request time #8164
Conversation
…and virtual modules
🦋 Changeset detectedLatest commit: 639470a The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
1c945da
to
06f6c65
Compare
cf. `read` callback in https://vitejs.dev/guide/api-plugin.html#handlehotupdate
I think I found (at least one) source of flakiness, which should be fixed by #8479 (or cherry-picked commit in this PR 92e3f40) Here is a quick summary:
This "empty" behavior is verified (though in a crude way) on CI log with this commit 89bfae5 /~https://github.com/remix-run/remix/actions/runs/7484569708/job/20371638696#step:7:463
Unfortunately, this "empty" issue should be fixed but there might be still a different source of flakiness (which might or might not related to this PR). For example, repeating 30 times can still hit a few errors though they are within the range of 3 retries. /~https://github.com/remix-run/remix/actions/runs/7485942421/job/20375315026#step:7:159 Now I reverted back all the debugging commits and the last CI run seems comparable to current dev branch's state. Please feel free to trigger a few more CI runs to see if this change is acceptable to merge. Thanks! (I split the change for #8479 but I'll leave the decision to your side either proceed separately or discard the other one and merge this directly) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for following up on the test flakiness. I've merged the latest code from dev and given it a bunch of CI runs and it's looking good to me.
🤖 Hello there, We just published version Thanks! |
🤖 Hello there, We just published version Thanks! |
Considering
resolvePluginConfig
depends only on route files andvite.config.ts
, I think it would make sense to use file watcher to trigger invalidation instead of request time (cf. #7908).Not in this PR yet, but I'm thinking to extend this approach to cover issues such as #7894, #8114.
If you want to tackle these issues together, then I can also continue within this PR, so please let me know.
(For now #8157 fixes #8114 based on
handleHotUpdate
, but this logic could be moved to watcher.)testing strategy
Currently there's no test to exercise route file addition/removal scenario, so I added a new test
integration/vite-dev-manifest-invalidation-test.ts
to verify that the server is aware of new route file addition.Note that client is still not aware of manifest change by itself, which is the same behavior reported in #7894.
(Currently windows/macos CI failing. Probably I'm missing some necessary path manipulation. I will investigate this further.)It seems it was a slight timing issue. I "fixed" tests by using
expect.poll
to do retry for small test block.