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

Dependencies optimization in ssr Environments during dev request handling can cause inconsistent states #19323

Open
7 tasks done
dario-piotrowicz opened this issue Jan 30, 2025 · 5 comments
Labels

Comments

@dario-piotrowicz
Copy link
Contributor

dario-piotrowicz commented Jan 30, 2025

Describe the bug

The problem consists in the fact that during local development dependency optimization (optimizeDeps) can happen, in an ssr environment, while a request is being processed.

This can be problematic as this allows modules to be reloaded while code is still being evaluated/run, meaning that the two can get out of sync.

Consider this very simplified example of such a scenario:

  • the vite dev server receives a request and it starts processing the request in an ssr environment
  • a dependency A is encountered and optimized
  • some global state gets saved based on the dependency initialization
  • a new dependency B is encountered (lazily)
  • so it gets optimized and a fullReload() occurs (which invalidates the environmen's whole module graph)
  • some code uses dependency A + the environment state with the assumption that the two match

The last point is where the issue occurs, the environment's state was modified, then the dependency A was reloaded and now the two might not match anymore, and this can be problematic for application.

The minimal reproduction linked in this issue should explain things further, it also adds some details and context as to why this is something that can become a common issue for many consumers/authors of ssr environments.

Reproduction

https://github.com/flarelabs-net/vite-dev-ssr-prebundling-mid-flight-issue-repro

Steps to reproduce

No response

System Info

System:
    OS: macOS 15.2
    CPU: (10) arm64 Apple M1 Pro
    Memory: 8.51 GB / 32.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 20.11.0 - ~/.nvm/versions/node/v20.11.0/bin/node
    Yarn: 1.22.19 - ~/.nvm/versions/node/v20.11.0/bin/yarn
    npm: 10.2.4 - ~/.nvm/versions/node/v20.11.0/bin/npm
    pnpm: 9.12.2 - ~/Library/pnpm/pnpm
    bun: 1.0.6 - ~/.bun/bin/bun
  Browsers:
    Chrome: 132.0.6834.111
    Safari: 18.2

Used Package Manager

pnpm

Logs

No response

Validations

@sapphi-red
Copy link
Member

I think this is the problem that was mentioned in #18358 (comment).

I think we need

  • an API that tell the environment to recreate / reset the execution environment
  • a way to tell the frameworks if the full-reload is needed (maybe this is not needed and can be handled internally)

@dario-piotrowicz
Copy link
Contributor Author

I think this is the problem that was mentioned in #18358 (comment).

ah yeah definitely, I completely forgot about that comment! yes I think that's exactly the same thing

I think we need

  • an API that tell the environment to recreate / reset the execution environment

We (or better @jamesopstad) already explored this sort of thing1, James has put in place some hacking to basically revert the request handling execution when the issue happens, this is however quite complex and brittle because although you can re-run the request that can still cause various side effects like logs2 and state changes (imagine for example a request that when handled changes the state of a local DB 😖).

So I think that something like this would be very unpractical 😓

  • a way to tell the frameworks if the full-reload is needed (maybe this is not needed and can be handled internally)

Isn't that already what happens? a full-reload even is emitted to the environment via the hot channel, isn't it?

environment.hot.send({
type: 'full-reload',
path: '*',
})

Footnotes

  1. For example we considered asking Vite to introduce a new error similar to ERR_OUTDATED_OPTIMIZED_DEP that could be try-catched by the environment request handling mechanism

  2. James had implemented a solution for logs, he's just cache the logs during request handling and only display them, all at once when the request is successfully handled, this is however both brittle (lots of edge cases can appear), non trivial to implement and doesn't provide a great dx for users (they don't get all logs real-time but all in one go when the request is handled)

@sapphi-red
Copy link
Member

We (or better @jamesopstad) already explored this sort of thing...

Since the new dep discovery only happens when executing a dynamic import (IIUC), would it help if the ERR_OUTDATED_OPTIMIZED_DEP like error happened directly after the dynamic import (before executing any content of the dynamic import)? That will make the new dep discovery error similar to a normal error that happens in normal code (e.g. errors caused by JSON.parse, sql connection error), we can probably rely on the normal error handlers (e.g. the transaction rollback).

The other way I can think of is:

  1. implement a module evaluator that supports ESM and CJS (internal ESModulesEvaluator)
    • I'm not sure if it works but I guess it can. Also I'm not sure if the behavior will be similar to the output bundle.
  2. externalize all CJS dependencies and handle it by the runtime itself, but still use the resolver from Vite
    • IIRC this was the path you were going initially.

Isn't that already what happens? a full-reload even is emitted to the environment via the hot channel, isn't it?

Yes. I mean, I think we need docs and some tests that covers the feature as I guess no frameworks will listen the event in the current state.

@dario-piotrowicz
Copy link
Contributor Author

would it help if the ERR_OUTDATED_OPTIMIZED_DEP like error happened directly after the dynamic import (before executing any content of the dynamic import)?

unfortunately I don't think so, because the issue is the amount of logic that gets run before the dynamic import is even encountered (let's say my dynamic import is in the middle of a side-effectfull function, the tricky part here is that the import is only encountered when half of the function has executed right?)

That will make the new dep discovery error similar to a normal error that happens in normal code (e.g. errors caused by JSON.parse, sql connection error), we can probably rely on the normal error handlers (e.g. the transaction rollback).

The difference is that there is no "real" error here (not one that the user should be aware of at least 😕)

The other way I can think of is:

  1. implement a module evaluator that supports ESM and CJS (internal ESModulesEvaluator)

yeah... although this feels pretty daunting... I feel like it might be best not to open such a can of worms 😓

  • I'm not sure if it works but I guess it can. Also I'm not sure if the behavior will be similar to the output bundle.
  1. externalize all CJS dependencies and handle it by the runtime itself, but still use the resolver from Vite

    • IIRC this was the path you were going initially.

Yes, we've also been considering that, but in my opinion it is better if we can still rely on ssr predundling as that really solves a lot of headaches on our side

Isn't that already what happens? a full-reload even is emitted to the environment via the hot channel, isn't it?

Yes. I mean, I think we need docs and some tests that covers the feature as I guess no frameworks will listen the event in the current state.

This actually made me think, maybe we do need to explore this angle more and make sure that the whole app (server environment + client environment) is reloaded, this would break HMR but only for this single instance (which is relatively rare during development)

@sapphi-red
Copy link
Member

The difference is that there is no "real" error here (not one that the user should be aware of at least 😕)

Yeah, that's true.

yeah... although this feels pretty daunting... I feel like it might be best not to open such a can of worms 😓

Yes, we've also been considering that, but in my opinion it is better if we can still rely on ssr predundling as that really solves a lot of headaches on our side

The other ways still using the optimizer would be to aggressively bundle dependencies instead of not bundling it until discovered.

  1. Crawl the modules beyond dynamic imports (that are statically analyzable)
    • In past, we had a discussion if we should do this (feat: scan free dev server #8319)). We didn't implement it as it would slow down the initial dev start, but maybe it makes sense in your case.
  2. Add all files in the src directory to optimizedDeps.entries. You'll have to add other directories if you're using a monorepo.
  3. Add all dependencies to optimizeDeps.include. This will require the user to restart the server if the code in the other package in the monorepo is changed.

The alternative would be to change the chunking strategy of the optimizer. The reason why the reload is needed is to load the changed chunk when a dependency was added. If each dependency (including transitive ones) is in a separate chunk, we don't need to reload. This would create many chunks and is not suitable for browsers, but could be an option for non-browser environments. Although I'm not sure if that's possible in a performant way and if it causes problems with circular references.

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

No branches or pull requests

2 participants