fix(vite): refactor module runner to module graph #18092
Conversation
|
|
| 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.
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.
Yes, I think having a urlToIdModuleMap would be good.
| @@ -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.
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.
| 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.
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.
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.
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.
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
moduleCacheinto a ModuleGraph that resembles what Vite already has on the server.This PR also add a new proposal to extend
import.metawith.environmentproperty: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.envfor environment variables. And it doesn't reference the actual serverenvironmentwhich 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 (moduleseems like a generic name that might be available there in some EcmaScript proposal)