Refactor module resolution logic to have configurable goal extensions #9430
Conversation
| "moduleResolution": "node", | ||
| "resolvedInputFiles": [ | ||
| "lib.d.ts", | ||
| "node_modules/@types/m1/index.d.ts", |
There was a problem hiding this comment.
So this file doesn't exist, but it's referenced?
There was a problem hiding this comment.
Hah. It didn't get committed because it's in a node_modules folder. That's funny. And annoying. Let me fix that.
There was a problem hiding this comment.
Yeah, see the last line I added to the gitignore file on my prior merge to master. That nearly got me too.
| jsonContent = jsonText ? <{ typings?: string, types?: string, main?: string }>JSON.parse(jsonText) : {}; | ||
| function getPackageEntry(packageJson: any, key: string, tag: string, state: ModuleResolutionState) { | ||
| const value = packageJson[key]; | ||
| if (typeof value === tag) { |
There was a problem hiding this comment.
when do we need something that is not a string?
There was a problem hiding this comment.
At this point, never. This was mostly @rbuckton's suggestion. I can specialize the function, if you'd prefer it.
There was a problem hiding this comment.
We discussed the possibility of augmenting the package.json for extensions, which could leverage this if we decided to lean in that direction.
|
This seems like a lot more code change than we need. I was thinking just something like the below in program.ts @ 781 would be sufficient, which is a minor change to existing logic. Anywhere this has issues? if (baseName !== "node_modules") {
// Try to load source from the package
const packageResult = loadModuleFromNodeModulesFolder(moduleName, directory, failedLookupLocations, state);
if (packageResult && hasTypeScriptFileExtension(packageResult)) {
// Always prefer TypeScript source (.ts, .tsx, .d.ts) shipped with the package
return packageResult;
}
else {
// Else prefer a types package over non-TypeScript results (e.g. JavaScript files)
const typesResult = loadModuleFromNodeModulesFolder(combinePaths("@types", moduleName), directory, failedLookupLocations, state);
if (typesResult || packageResult) {
return typesResult || packageResult;
}
}
} |
|
I just tested the above simple change and that seems to work fine too. I'll push some commits to the (slightly) related change at #9421 for review. |
| @@ -1134,10 +1134,33 @@ namespace ts { | |||
| */ | |||
| export const supportedTypeScriptExtensions = [".ts", ".tsx", ".d.ts"]; | |||
| export const supportedJavascriptExtensions = [".js", ".jsx"]; | |||
| const allSupportedExtensions = supportedTypeScriptExtensions.concat(supportedJavascriptExtensions); | |||
|
|
|||
| export function getExtensionsForGoal(goal: ResolutionGoal): string[] { | |||
There was a problem hiding this comment.
ResolutionGoal seems to have some similarities to the ExtensionPriority enum used as part of glob resolution.
There was a problem hiding this comment.
Merged! ExtensionPriority overlays nicely onto what I used to call ResolutionGoal (now since renamed).
|
@billti I have ulterior motives for making this a little more complicated: I need to make it easy (in the near future) to specify that I want to load js files only for resolving modules in the extension PR (which is where the first draft of this code came from). Additionally, we were passing a handful of flags around to control the loading of js and jsx/tsx - this now unifies them. And, thanks to a little bit more effort, is now merged with the |
|
Right, I was figuring as much. But we need this fix for 2.0, which has already snapped for stabilization. I'd rather keep it as simple as possible, especially as the extensibility stuff is very early on at this point and may go through more changes. That said, I'll let @mhegazy make the call on which pull request to take. |
|
On a related note, Travis finally finished testing this PR and it's now lit up green. :D |
weswigham
changed the title
|
@mhegazy Now that we've cut a |
|
@vladima Could you look at this PR? |
|
This is stale by now. sorry for the delay. closing. |
Fixes #9427