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(vite)!: refactor "module cache" to "evaluated modules", pass down module to "runInlinedModule" #18092

Merged
merged 20 commits into from
Sep 25, 2024

Conversation

sheremet-va
Copy link
Member

@sheremet-va sheremet-va commented Sep 12, 2024

Description

This PR refactors vite-node's moduleCache into EvaluatedModules that resembles what Vite already has on the server.


This is not part of the PR as of September 23, it will be a separate discussion.

This PR also add a new proposal to extend import.meta with .environment property:

// this is only available for modules that run inside the module runner

import.meta.environment.module // current module
import.meta.environment.moduleGraph // access to module graph

// or
import.meta.hot.module // current module
import.meta.hot.moduleGraph // access to module graph

The goal here is to have easy access to the module graph.

I am personally not sold on the "environment" name - we already have import.meta.env for environment variables. And it doesn't reference the actual server environment which makes it a bit confusing. This is more of a "module context" - or maybe we don't even need a single property to hold this, and we can just put in on import.meta, but I am not sure how future-proof it is (module seems like a generic name that might be available there in some EcmaScript proposal)

Copy link

stackblitz bot commented Sep 12, 2024

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

Copy link

@jamesopstad jamesopstad left a comment

Choose a reason for hiding this comment

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

Just a few minor observations/questions but looks good. Thanks.

packages/vite/src/module-runner/hmrHandler.ts Outdated Show resolved Hide resolved
packages/vite/src/module-runner/hmrHandler.ts Outdated Show resolved Hide resolved
packages/vite/src/module-runner/moduleCache.ts Outdated Show resolved Hide resolved
packages/vite/src/module-runner/moduleCache.ts Outdated Show resolved Hide resolved
packages/vite/src/module-runner/runner.ts Outdated Show resolved Hide resolved
packages/vite/src/module-runner/runner.ts Outdated Show resolved Hide resolved
@sheremet-va sheremet-va marked this pull request as ready for review September 23, 2024 08:52
/**
* Invalidate modules that dependent on the given modules, up to the main entry
*/
invalidateDepTree(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was using invalidateDepTree in my example for rsc environment's node runner https://github.com/hi-ogawa/vite-environment-examples/blob/edf04cf23fb472a9d085e1a424ae7543164b98f8/examples/react-server/vite.config.ts#L173 but it doesn't look necessary since it's same as default non-hmr invalidation, so I just removed it hi-ogawa/vite-environment-examples#109

@patak-dev
Copy link
Member

/ecosystem-ci run

patak-dev
patak-dev previously approved these changes Sep 24, 2024
Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

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

We discussed that we may want to rename runner.moduleGraph to runner.evaluatedModules to avoid confusions between two module graphs being different. Good to do this later though.

@sheremet-va sheremet-va changed the title fix(vite): refactor module runner to module graph fix(vite): refactor "module cache" to "evaluated modules" Sep 24, 2024
@vite-ecosystem-ci
Copy link

@patak-dev
Copy link
Member

/ecosystem-ci run nuxt

@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on 5a04b5d: Open

suite result latest scheduled
nuxt failure success

@hi-ogawa
Copy link
Collaborator

My example's prepareStackTrace on workerd seems to be broken (ecosystem ci shows it and I'm also checking with local build). Do you know if this is something on my side (or workerd runtime itself)?

@sheremet-va
Copy link
Member Author

sheremet-va commented Sep 25, 2024

My example's prepareStackTrace on workerd seems to be broken (ecosystem ci shows it and I'm also checking with local build). Do you know if this is something on my side (or workerd runtime itself)?

Previously, createNodeEnvironment would create an environment that assumed the usage of ESModulesEvaluator and was always adding two lines to the source map. Now it is done automatically by ESModulesEvaluator with startOffset property. If there is no startOffset, lines are not added. Does workerd require this?

@hi-ogawa
Copy link
Collaborator

hi-ogawa commented Sep 25, 2024

startOffset doesn't seem related. I have a fix/workaround in hi-ogawa/vite-environment-examples#111 and it looks like what changed is Workerd's original stack which is created based on id of runInlinedModule(..., id) is not rewritten any more. For example, when id = "/src/crash-ssr.ts", previously module runner would rewrite this to /(root)/src/crash-ssr.ts but now I need to add (root) myself to the original stack.

@sheremet-va
Copy link
Member Author

sheremet-va commented Sep 25, 2024

startOffset doesn't seem related. I have a fix/workaround in hi-ogawa/vite-environment-examples#111 and it looks like what changed is Workerd's original stack which is created based on id of runInlinedModule(..., id) is not rewritten any more. For example, when id = "/src/crash-ssr.ts", previously module runner would rewrite this to /(root)/src/crash-ssr.ts but now I need to add (root) myself to the original stack.

JSDoc says that "id" is "ID that was used to fetch the module", not the module ID. Technically, it's a "url".

Why don't you use context[ssrImportMetaKey].url?

I guess we can just pass down the ModuleNode there for easier access

@hi-ogawa
Copy link
Collaborator

JSDoc says that "id" is "ID that was used to fetch the module", not the module ID. Technically, it's a "url".

Why don't you use context[ssrImportMetaKey].url?

I guess we can just pass down the ModuleNode there for easier access

Good point, using context.__vite_ssr_import_meta__.filename works hi-ogawa/vite-environment-examples#111. I think maybe we have discussed the 3rd argument id is for stacktrace at some point, so it looks like I've using it since vite runtime days.

@sheremet-va sheremet-va changed the title fix(vite): refactor "module cache" to "evaluated modules" fix(vite)!: refactor "module cache" to "evaluated modules", pass down module to "runInlinedModule" Sep 25, 2024
@sheremet-va
Copy link
Member Author

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

@sheremet-va
Copy link
Member Author

Seems like the errors are the same as before the PR

@patak-dev
Copy link
Member

Seems like the errors are the same as before the PR

Nuxt seems to be passing in main: https://github.com/vitejs/vite-ecosystem-ci/actions/runs/11031700332/job/30639019630 🤔

@sheremet-va
Copy link
Member Author

Seems like it's just flaky because it worked yesterday after a rerun: #18092 (comment)

The changes after that can't affect Nuxt since it doesn't use these APIs

Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

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

Ok to merge this now and deal with the Nuxt issue later if it is unrelated. The linked comment shows that it was failing though (and I tried twice in main and it worked both times 🤔)

@patak-dev patak-dev merged commit e83beff into vitejs:main Sep 25, 2024
13 checks passed
@patak-dev
Copy link
Member

(removed the ! as it isn't a breaking change against Vite 5)

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

Successfully merging this pull request may close these issues.

4 participants