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

perf(remix-dev/vite): use fs watcher to invalidate manifest instead of request time #8164

Merged

Conversation

hi-ogawa
Copy link
Contributor

@hi-ogawa hi-ogawa commented Nov 29, 2023

Considering resolvePluginConfig depends only on route files and vite.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.

Copy link

changeset-bot bot commented Nov 29, 2023

🦋 Changeset detected

Latest commit: 639470a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 16 packages
Name Type
@remix-run/dev Patch
create-remix Patch
remix Patch
@remix-run/architect Patch
@remix-run/cloudflare Patch
@remix-run/cloudflare-pages Patch
@remix-run/cloudflare-workers Patch
@remix-run/css-bundle Patch
@remix-run/deno Patch
@remix-run/eslint-config Patch
@remix-run/express Patch
@remix-run/node Patch
@remix-run/react Patch
@remix-run/serve Patch
@remix-run/server-runtime Patch
@remix-run/testing Patch

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

@hi-ogawa hi-ogawa marked this pull request as ready for review November 29, 2023 09:09
@hi-ogawa hi-ogawa changed the title refactor(remix-dev/vite): use fs watcher to invalidate plugin config and virtual modules refactor(remix-dev/vite): use fs watcher to trigger manifest invalidation Nov 30, 2023
@hi-ogawa hi-ogawa marked this pull request as draft November 30, 2023 07:50
@hi-ogawa hi-ogawa force-pushed the refactor-vite-resolvePluginConfig branch from 1c945da to 06f6c65 Compare November 30, 2023 07:52
@hi-ogawa hi-ogawa marked this pull request as ready for review November 30, 2023 09:01
@hi-ogawa
Copy link
Contributor Author

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:

  • Flakiness was observed exclusively on Linux machine (namely both ubuntu/chromium and ubuntu/firefox) on CI.
  • I use ArchLinux for dev but I couldn't reproduce it locally.
  • When reading file via fs.readFile during handleHotUpdate, it sometimes ends up empty as explained in https://vitejs.dev/guide/api-plugin.html#handlehotupdate and it suggests to use HmrContext.read to avoid such file system io inconsistency.

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

## during handleHotUpdate  "sourceExports" became empty (it should at least have "default")
## and ends up with `hasLoader` flag going back to false

[getRouteMetadata:empty] routes/_index.tsx

[handleHotUpdate:invalidateVirtualModules] {
  file: '/home/runner/work/remix/remix/.tmp/integration/remix-4g4re5itv9g/app/routes/_index.tsx',
  oldRouteMetadata: {
    ...
    hasLoader: true,
    ...
  },
  newRouteMetadata: {
    ...
    hasLoader: false,
    ...
  }
}

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)

Copy link
Member

@markdalgleish markdalgleish left a 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.

@markdalgleish markdalgleish merged commit a1c376a into remix-run:dev Jan 12, 2024
9 checks passed
@hi-ogawa hi-ogawa deleted the refactor-vite-resolvePluginConfig branch January 12, 2024 03:44
Copy link
Contributor

🤖 Hello there,

We just published version 2.5.1-pre.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Copy link
Contributor

🤖 Hello there,

We just published version 2.5.1 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants