Api cleanup for Module and Type Reference directive resolution#51546
Conversation
ab63fbf to
7c04a8c
Compare
7c04a8c to
ed98e24
Compare
… before solving, so ResolutionCache doesnt need this check
… doesnt update own options
…mon scenarios so cache can be shared between redirects
…ookupLocations Also make accidental public failed lookup internal
…gnostics so they can be reused when program is reused
ed98e24 to
16846c8
Compare
| }, | ||
| "failedLookupLocations": [], | ||
| "affectingLocations": [], | ||
| "resolutionDiagnostics": [] |
There was a problem hiding this comment.
I always assumed these were non-optional for V8 monomorphism reasons (though I would expect undefined instead of an empty array might be the best for performance?). @DanielRosenwasser @rbuckton is there a guiding principle for how to approach these things? Does it even make sense to try to make optimizations without looking at data from deoptigate?
There was a problem hiding this comment.
Made these optional because we want to handle properties as optional (from API perspective) We can always set these to empty array like before but thought of setting to undefined instead of empty array would better (since it is mutable empty array so always new array)
There was a problem hiding this comment.
Yeah, I’m sure the new arrays is the worst of all the options. There might be some benefit to initializing to undefined but I’m not sure.
| containingSourceFile: SourceFile, | ||
| reusedNames: readonly StringLiteralLike[] | undefined, | ||
| ): readonly ResolvedModuleWithFailedLookupLocations[]; | ||
| resolveTypeReferenceDirectiveReferences?<T extends FileReference | string>( |
There was a problem hiding this comment.
It at least makes sense, despite the repetition - you need actual references to type reference directives now, rather than strings (ideally). Related: Like the module name version, should we split the signature and attempt to deprecate the string-handling form?
There was a problem hiding this comment.
Yeah, the thing that makes me dislike the name is that it still takes a string, so seems like a bit of a misnomer.
andrewbranch
left a comment
There was a problem hiding this comment.
Looks good to me overall, but I think others should weigh in on the API changes. I also suspect we have some other infrastructure that might use sourceFile.resolvedModules even though it’s internal... @sandersn and/or I should check DefinitelyTyped-tools.
weswigham
left a comment
There was a problem hiding this comment.
Better unifying the mechanisms for type reference directive loading and import loading is 👍
|
I checked some other repos for references to the changed internal APIs, and couldn’t find anything that will be an issue. |
CompilerHost,LanguageServiceHostandProgramHostwhich is used bywatchandtsc --build. ce8da01 12a16ce