-
-
Notifications
You must be signed in to change notification settings - Fork 6.1k
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 runner to module graph #18092
base: main
Are you sure you want to change the base?
Conversation
Run & review this pull request in StackBlitz Codeflow. |
const id = posixResolve(this.root, url.slice(1)) | ||
return this.idToModuleMap.get(id) | ||
} | ||
return this.idToModuleMap.get(url) |
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.
We probably need to store the url
map still. For example, this import will load the same module, but getModuleByUrl
won't return the module for one of them:
await import('./module')
const moduleName = './module'
await import(moduleName)
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.
Yes, I think having a urlToIdModuleMap
would be good.
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.
Just a few minor observations/questions but looks good. Thanks.
@@ -122,9 +122,9 @@ class Queue { | |||
|
|||
function getModulesByFile(runner: ModuleRunner, file: string) { | |||
const modules: string[] = [] | |||
for (const [id, mod] of runner.moduleCache.entries()) { | |||
for (const [_, mod] of runner.moduleGraph.idToModuleMap.entries()) { |
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.
We could use values
here as we don't need the key.
for (const [_, mod] of runner.moduleGraph.idToModuleMap.entries()) { | |
for (const mod of runner.moduleGraph.idToModuleMap.values()) { |
@@ -155,9 +158,9 @@ function findAllEntrypoints( | |||
runner: ModuleRunner, | |||
entrypoints = new Set<string>(), | |||
): Set<string> { | |||
for (const [id, mod] of runner.moduleCache.entries()) { | |||
for (const [_, mod] of runner.moduleGraph.idToModuleMap.entries()) { |
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.
for (const [_, mod] of runner.moduleGraph.idToModuleMap.entries()) { | |
for (const mod of runner.moduleGraph.idToModuleMap.values()) { |
return super.set(modulePath, mod) | ||
} | ||
public idToModuleMap = new Map<string, ModuleRunnerNode>() | ||
public fileToModuleMap = new Map<string, ModuleRunnerNode[]>() |
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.
This should be named fileToModulesMap
. Should we also use Set<ModuleRunnerNode>
rather than ModuleRunnerNode[]
for consistency with EnvironmentModuleGraph
?
const id = posixResolve(this.root, url.slice(1)) | ||
return this.idToModuleMap.get(id) | ||
} | ||
return this.idToModuleMap.get(url) |
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.
Yes, I think having a urlToIdModuleMap
would be good.
@@ -55,7 +55,10 @@ export class ModuleRunner { | |||
private readonly transport: RunnerTransport | |||
private readonly resetSourceMapSupport?: () => void | |||
private readonly root: string | |||
private readonly moduleInfoCache = new Map<string, Promise<ModuleCache>>() | |||
private readonly moduleInfoCache = new Map< |
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.
Is moduleInfo
the right name for this? It seems a bit confusing as ModuleInfo
is a type that has a different meaning elsewhere.
depMod.importers!.add(moduleId) | ||
mod.imports!.add(depMod.id) |
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.
These non-nullish assertions aren't needed.
depMod.importers!.add(moduleId) | |
mod.imports!.add(depMod.id) | |
depMod.importers.add(moduleId) | |
mod.imports.add(depMod.id) |
Description
This PR refactors vite-node's
moduleCache
into a ModuleGraph that resembles what Vite already has on the server.This PR also add a new proposal to extend
import.meta
with.environment
property: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 serverenvironment
which makes it a bit confusing. This is more of acontext
- or maybe we don't even need a single property to hold this, and we can just put in onimport.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)