-
Notifications
You must be signed in to change notification settings - Fork 8
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
Optimize compiler filesystem caching #16
base: main
Are you sure you want to change the base?
Optimize compiler filesystem caching #16
Conversation
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.
Mostly nitpicking because a lot of these changes are functionally not doing anything and I assume they were at some point introduced during debugging.
The only thing that I'd ask you to look into would be if this would affect watch mode, and if it does we probably need to make the caching configurable.
const filenames = Object.keys(files); | ||
this.compilerHost.setScriptFileNames(filenames); |
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.
I think there's a bunch of these going on for logging purposes - could you undo them so we have the diff a little bit smaller?
const result = | ||
!!virtualFiles[normalize(fileName)] || ts.sys.fileExists(fileName); | ||
return result; |
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.
Also pulled apart for logging purposes I guess?
if (normalized in cachedFiles) { | ||
const cacheResult = cachedFiles[fileName]; | ||
|
||
if (cacheResult) { | ||
return cacheResult; | ||
} | ||
} | ||
|
||
const contents = ts.sys.readFile(fileName); | ||
|
||
if (contents) { | ||
cachedFiles[normalized] = contents; | ||
} |
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.
Did you verify that this doesn't cause problems in watch mode?
If it does, we might need a flag to swap behaviours here.
@@ -166,7 +190,7 @@ function createCompilerHost( | |||
); | |||
}, | |||
resolveModuleNames(moduleNames, containingFile) { | |||
return moduleNames.map((moduleName) => { | |||
const mappedModules = moduleNames.map((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.
This could also stay a direct return and was only pulled apart for logging I think?
const resolvedModule = ts.resolveModuleName( | ||
newModuleName, | ||
containingFile, | ||
compilerOptions, | ||
this | ||
).resolvedModule; | ||
|
||
return resolvedModule; |
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.
const resolvedModule = ts.resolveModuleName( | |
newModuleName, | |
containingFile, | |
compilerOptions, | |
this | |
).resolvedModule; | |
return resolvedModule; | |
return ts.resolveModuleName( | |
newModuleName, | |
containingFile, | |
compilerOptions, | |
this | |
).resolvedModule; |
|
||
let cachedFiles: Record<string, string | undefined> = {}; | ||
|
||
const host: ModifiedCompilerHost = { |
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.
I think this could also stay a direct return?
This PR: