Cypress's Vite Dev Server exits when bundling cjs on the fly

JessicaSachs commented 2 years ago

Describe the bug

Disclaimer: @patak-dev told me to put this issue in here rather than in Cypress's repo to start. I understand this issue is likely framework-specific. Happy to transfer it whenever necessary.

Vite x Cypress for Component Testing

👋🏻 Hello, I'm the author of @cypress/vite-dev-server, a package I wrote around March 2021 for use with the Cypress Component Test Runner. Until Vitest, using Cypress was practically the only way to test Vue/React components written and bundled with Vite.

The first work-around

In preparation for releasing Cypress 10, we've adopted Vite internally and have been heavy users of Vite over the last 8 months. Early on, we noticed that if Vite discovered a non-ESM module, it would reload the page and this would break Cypress when we used our Vite Dev Server plugin. It would fail to compile the cjs code and crash. We came up with a work-around:

We were able to ask users to use @antfu's vite-plugin-optimize-persist and vite-plugin-package-config plugins to get a working dev server.

Using optimizeDeps kind of worked

Unfortunately, as we're scaling up and integrating into other projects, we've found that those two plugins aren't enough, and we're seeing additional edge cases that are causing instability. We were treating one of the symptoms of Cy + Vite's instability. The Cy dev server wasn't crashing immediately, but there's a deeper issue surrounding HMR and full page reloads. For example, in projects that use unocss, the way the styles are pushing onto the client causes another full page reload.

Reproduction

I'm currently blocked from merging a PR to add Cypress Component Testing for Vitest's UI because of the flake. https://github.com/vitest-dev/vitest/pull/590

Cypress's own monorepo has this issue and a 100% reproducible test case, but working in the Cypress monorepo can be a bit of a challenge. Here are some instructions...

Clone the main Cypress repo on the 10.0-release branch
yarn in the root of the repository
cd packages/app
rm -rf node_modules/.vite && yarn cypress:run:ct --browser chrome --headed --config video=false --spec **/SettingsContainer.cy.tsx

As a last resort...

One of the options we have is to switch to using vite build and statically serving the dist'd files. We don't want to do this for a number of reasons: production parity, loss of HMR, and changes in compilation options (for example, Vitesse's SSG requires <ClientOnly> on vite build, but not on vite dev. We want consistency between build environments).

Call for help

I'd really appreciate guidance and discussion around how to improve the stability of Cypress Component Testing with Vite.

Steps to reproduce

Checkout the PR on Vitest
pnpm install in the root of the project
cd packages/ui
rm -rf ./node_modules/.vite
pnpm ci

Afterwards, you can check the packages/ui/cypress/videos directory to see a video of the failed test.

When you re-run the test, it will pass (some of the time) because the packages have been prebundled. To reproduce, make sure you're running rm -rf ./node_modules/.vite before running pnpm ci

Reproduction

I wasn't able to minimize the reproduction further than the already broken PR in Vitest: https://github.com/vitest-dev/vitest/pull/590

System Info

System:
    OS: macOS 11.4
    CPU: (16) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
    Memory: 3.06 GB / 32.00 GB
    Shell: 5.8 - /bin/zsh
  Binaries:
    Node: 14.17.0 - ~/.nvm/versions/node/v14.17.0/bin/node
    Yarn: 1.22.10 - /usr/local/bin/yarn
    npm: 6.14.13 - ~/.nvm/versions/node/v14.17.0/bin/npm
  Browsers:
    Chrome Canary: 99.0.4840.0
    Firefox: 95.0
    Safari: 14.1.1
  npmPackages:
    @vitejs/plugin-vue: ^2.0.1 => 2.0.1 
    @vitejs/plugin-vue-jsx: ^1.3.3 => 1.3.3

Used Package Manager

pnpm

Logs

😅 if there were instructions for how to turn --debug logs on from within the JavaScript API, that'd be great.

Validations

[X] Follow our Code of Conduct
[X] Read the Contributing Guidelines.
[X] Read the docs.
[X] Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
[X] Make sure this is a Vite issue and not a framework-specific issue. For example, if it's a Vue SFC related bug, it should likely be reported to https://github.com/vuejs/vue-next instead.
[X] Check that this is a concrete bug. For Q&A open a GitHub Discussion or join our Discord Chat Server.
[X] The provided reproduction is a minimal reproducible example of the bug.

lmiller1990 commented 2 years ago

Vite and Cypress, the dream. Let me know if I can do anything to help!

brian-mann commented 2 years ago

If its helpful - here is a screenshot of filtered network requests. As @JessicaSachs said - when there is a request to the entrypoint, vite serves the file but then immediately sends a "full page refresh" signal so in the network requests you will see the same requests made for the same files. Cypress does not expect a spec to be loaded in, and then immediately unload and load. We could work around this in core, but hopefully there is a better approach we could take directly in vite.

Screenshot 10 -01

patak-dev commented 2 years ago

Adding context from Evan, that @JessicaSachs shared when we discussed about it last time:

The reason it needs to reload is because the dep may have cross dependencies with other already optimized deps - so to ensure correctness we have to re-run the whole pre-bundle pass and reload to invalidate imports to the other deps

At that point, @JessicaSachs, you mentioned that you will like to try to make cypress resilient to page reloads. We could check what events are needed for you to be able to implement this. Vite could send a fully-loaded event to the client once everything is done, but what does this mean when dealing with dynamic imports and the user interacting with the page while it is being loaded?

I don't think that full-page reloads is a good DX here, but I don't understand exactly the cross dependencies issue that forces Vite to issue a full reload. The code is here if others want to check: https://github.com/vitejs/vite/blob/17339558979f02ba6e44f8b9a59ae2717224c96c/packages/vite/src/node/optimizer/registerMissing.ts#L86 Maybe there is a way to rework this by accepting different tradeoffs

patak-dev commented 2 years ago

Maybe there could be an option in Vite to accept that missed dependencies aren't going to be optimized? (or they can be queued for the next full page reload, instead of forcing it). Here you can see a node dependency that isn't yet optimized and so it is registered as missing: https://github.com/vitejs/vite/blob/fb7ba5353bb35d41f2ec2c90b6b8e5fecd508d07/packages/vite/src/node/plugins/resolve.ts#L599 Currently, this means that it will return synchronously the path to node_modules, and enqueue and optimizeDeps step that will end up in a full reload because IIUC your page will be pointing to the original dep in node_modules instead of the optimized path that leaves in the cacheDir (by default node_modules/.vite). Maybe we could continue using the original dep for this run... and have a deps queue in the cache. Next time optimizeDeps is called on server start, it also reads these missing deps and processes them. The first-page load will be slower, but there will not be full reloads needed. Just thinking out loud here.

JessicaSachs commented 2 years ago

So... maybe an aside, but I'm genuinely wondering how Vitest handles this issue.

JessicaSachs commented 2 years ago

More of a meta discussion for another time, but I think that Vitest and Cy's Vite support should be as === as possible so that swapping between headless/headed tests doesn't have any foot-guns WRT module loading, deps, and execution context.

JessicaSachs commented 2 years ago

Maybe we could continue using the original dep for this run... and have a deps queue in the cache. Next time optimizeDeps is called on server start, it also reads these missing deps and processes them. The first-page load will be slower, but there will not be full reloads needed. Just thinking out loud here.

So IIUC, this would cause the first page that encounters a dependency to be slower to load, but subsequent pages/reloads would be at Vite-speed. Yes?

patak-dev commented 2 years ago

So... maybe an aside, but I'm genuinely wondering how Vitest handles this issue.

I think it directly disables deps optimization? https://github.com/vitest-dev/vitest/commit/e379f64a834f314d9d68b38e43d2307db423a4d3#diff-0196708c26b0c02a910c665662539b335e23bf22c303bae9a1b28b3fa1b64cf6 There isn't a browser in the middle requesting thousands of files for lodash, so I think it just gets in the way there. You may need to ask Anthony about it.

patak-dev commented 2 years ago

Maybe we could continue using the original dep for this run... and have a deps queue in the cache. Next time optimizeDeps is called on server start, it also reads these missing deps and processes them. The first-page load will be slower, but there will not be full reloads needed. Just thinking out loud here.

So IIUC, this would cause the first page that encounters a dependency to be slower to load, but subsequent pages/reloads would be at Vite-speed. Yes?

Yes, and I don't know if the first page will really be that much slower, or it may be even faster? Because you are comparing loading with a few deps unoptimized against loading with possible several full page reloads in the middle so the last reload has all of the deps optimized? I wonder if the later isn't slower, and this shouldn't be an option but the default

JessicaSachs commented 2 years ago

Maybe we could continue using the original dep for this run... and have a deps queue in the cache. Next time optimizeDeps is called on server start, it also reads these missing deps and processes them. The first-page load will be slower, but there will not be full reloads needed. Just thinking out loud here.

KK. Gonna prototype this right now.

JessicaSachs commented 2 years ago

TL;DR so far

👋🏻 It's been a big struggle to get the environment up. That's what I spent most of my day doing -- working toward the goal of being able to prototype the suggestions that @patak-dev and I spoke about.

What I need is a test fixture or project/page that has many optimizeDeps cache misses and ends up responding with a lot of 408s. Ultimately, triggering this block of code:

https://github.com/vitejs/vite/blob/fb7ba5353bb35d41f2ec2c90b6b8e5fecd508d07/packages/vite/src/node/server/middlewares/transform.ts#L66-L94

Next steps

I will work on this tomorrow to try and simplify the reproduction further and move it out of Cypress-land. If someone has a simplified project I can use that triggers the failure state I'm talking about, that would be super helpful.

For a better demonstration of the symptoms we're seeing, I've attached a video. It's split into two parts. Essentially we're getting a ton of 408's, reloads, and marvelous failures.

The initial failure. This has like 4 reloads.

https://user-images.githubusercontent.com/2801156/151488655-5667e289-1afc-44bc-9780-6cf2796090de.mov

Success when reloading

https://user-images.githubusercontent.com/2801156/151488882-cdd38ec7-c8e3-4931-b4ba-3c444e0e2e1a.mov

This only really happens for the first build of the first page for a user's particular machine when the project is large enough

So, basically, most CI runs.

Project Info More details about the project under here, including information for where to find the Vite configs and plugin code written.

The Vite config used for the page in the video is [here in the Cypress App -- the web frontend](https://github.com/cypress-io/cypress/blob/10.0-release/packages/app/vite.config.ts), but most of the config is within the [frontend-shared](https://github.com/cypress-io/cypress/blob/10.0-release/packages/frontend-shared/vite.config.ts) package's Vite config. ## Package Arch The Cypress + Vite Dev Server code lives [here](https://github.com/cypress-io/cypress/tree/10.0-release/npm/vite-dev-server/src). The architecture is stupidly simple, we create a Vite dev server from the JavaScript API and serve each spec file as a "page" in Vite. Everything functions fine outside of the full-page refresh behavior for the first spec.

Thanks for the help so far. I'm around to Discord for sync comms. My plan is to keep this issue updated at least daily because solving this is really critical to Cy and Cy's upcoming release.

Cheers, Jess

JessicaSachs commented 2 years ago

@patak-dev and I spent a large portion of the morning pairing and working on an isolated reproduction within the Optimize Missing Deps playground. I learned how to develop in Vite, run the tests, and we were able to begin spiking on a possible fix.

We're intending to add a mode for blocking requests that require dependency resolution instead of enqueueing them. For Cypress specifically we believe it will significantly improve the performance.

We want to conditionally make this line blocking:

https://github.com/vitejs/vite/blob/main/packages/vite/src/node/plugins/resolve.ts#L599

Such that network requests for unoptimized modules do not queue and push a refresh when they are optimized, but instead synchronously perform the work and do not reload.

Next: I'm in the middle of the spike and now that I have a reproduction and feel more comfortable developing + running the Vite test suite, I'll continue spiking and will post additional updates as I have them.

JessicaSachs commented 2 years ago

Final update for now.

@patak-dev asked me to keep this issue open as a placeholder to discuss an architecture that does not have the below flaws and resolves all dependencies AND transient dependencies such that incremental reloads are not necessary.

Update on the previous approach:

We've found the fatal flaw in the above strategy which is that when resolving dependencies, shared transient ones may resolve to a different version. (This is what Evan told me in October, lol, I just didn't fully understand how limited we were until now.)
If we were to push up a new version for a transitive dep, we would have multiple versions of a dependency running within the same page context.
Having multiple versions of certain packages running at the same time can cause very critical application errors (e.g. React can't run multiple versions on one page).
Additionally, without doing a full-page reload, any side effects would not be cleaned up.

Therefor, unless we know all of the transient dependencies for all entries, we cannot safely prevent a full page reload.

Cypress's decision is to use `vite build` + `vite serve` in CI. This will reliably solve the issue for fresh machines that have never run the user's application before.

For local development, we will continue to use vite dev to benefit from all of the debug and development utilities that plugins like WindiCSS provide to users.

The surface area of risk for local development is restricted to users who have:

Never run their application before
Never run their tests before
Deleted their cacheDir

In all of these local-only edge cases where a user has a completely clean cache, a simple Cmd + R will fix this issue.

Timeline

After the Cypress v10 launch, we will improve the DX for users developing locally with cypress open.
Immediately (today), we will make @cypress/vite-dev-server utilize vite build + vite serve when running Cypress in run mode (AKA, CI). This will fix the majority of the instability for all versions of Cypress that can run Component Testing v7+.

Future

If the Vite team ever has a way to turn off automatic reloads, that would allow us to create a more consistent experience between dev and prod.

patak-dev commented 2 years ago

Thanks for the write up, lets keep this open. We may find ways to at least avoid some full reloads later.

A detail about the new CI approach. By vite serve I think you mean vite preview . Take into account the preview server is only for debugging locally, it may work for your CI case but it may be better to serve the build with a diff production ready server

JessicaSachs commented 2 years ago

Thanks for the write up, lets keep this open. We may find ways to at least avoid some full reloads later.

A detail about the new CI approach. By vite serve I think you mean vite preview . Take into account the preview server is only for debugging locally, it may work for your CI case but it may be better to serve the build with a diff production ready server

Ah sorry I was thinking about vitepress's serve command. Yes. We'll use an express server, then. Cy already has one up.

patak-dev commented 2 years ago

@JessicaSachs https://github.com/vitejs/vite/pull/6758 may be able to help with this issue. Let's talk and see if we can test the PR against Cypress.

francisu commented 2 years ago

@JessicaSachs I read through this with interest as I think I'm hitting this same problem when using Vite and cypress run-ct. When running the first time, vite start has to figure out the dependencies, and this is triggered on the first test execution.

I was curious about your use of vite build and vite preview in CI. Does this mean you are able to execute the component tests from a vite preview environment without using the dev server? If so, can you point me to how this works (I looked through the docs and could not find anything).

My strategy for now is just to use run-ct on a single test case (which will likely fail -- I will ignore the failure), but this after this point, vite will have cached everything so the real run-ct will work.

A better solution is welcome.

patak-dev commented 2 years ago

The cold start experience has been improved after https://github.com/vitejs/vite/pull/6758, and related PRs. These changes will be part of 2.9. Discussed with @JessicaSachs that confirmed that the 2.9 beta is working better for Cypress. Let's close this issue, please open new ones if needed.

vitejs / vite