Skip to content

Conversation

@markerikson
Copy link
Contributor

This PR:

  • Optimizes the compiler wrapping by caching results that were getting re-requested numerous times:
    • Resolved TS modules
    • File source contents
  • Tweaks the module resolution logic for "files" that are part of the snippet codeblocks with relative imports - specifying an absolute path seemed to help

Copy link
Owner

@phryneas phryneas left a 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.

Comment on lines +61 to +62
const filenames = Object.keys(files);
this.compilerHost.setScriptFileNames(filenames);
Copy link
Owner

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?

Comment on lines +127 to +129
const result =
!!virtualFiles[normalize(fileName)] || ts.sys.fileExists(fileName);
return result;
Copy link
Owner

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?

Comment on lines +139 to +151
if (normalized in cachedFiles) {
const cacheResult = cachedFiles[fileName];

if (cacheResult) {
return cacheResult;
}
}

const contents = ts.sys.readFile(fileName);

if (contents) {
cachedFiles[normalized] = contents;
}
Copy link
Owner

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.

},
resolveModuleNames(moduleNames, containingFile) {
return moduleNames.map((moduleName) => {
const mappedModules = moduleNames.map((moduleName) => {
Copy link
Owner

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?

Comment on lines +220 to +227
const resolvedModule = ts.resolveModuleName(
newModuleName,
containingFile,
compilerOptions,
this
).resolvedModule;

return resolvedModule;
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
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 = {
Copy link
Owner

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?

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.

2 participants