Dont emit module resolution errors when looking up specifiers for container symbols #28558
Conversation
There was a problem hiding this comment.
Code change looks good, and I support not breaking people. Make sure @DanielRosenwasser signs off too, though, since he's kind of in charge of breaking (or not-breaking) changes.
| function resolveExternalModuleName(location: Node, moduleReferenceExpression: Expression): Symbol | undefined { | ||
| return resolveExternalModuleNameWorker(location, moduleReferenceExpression, Diagnostics.Cannot_find_module_0); | ||
| function resolveExternalModuleName(location: Node, moduleReferenceExpression: Expression, ignoreErrors?: boolean): Symbol | undefined { | ||
| return resolveExternalModuleNameWorker(location, moduleReferenceExpression, ignoreErrors ? undefined : Diagnostics.Cannot_find_module_0); |
There was a problem hiding this comment.
Do you need to handle this at https://github.com/Microsoft/TypeScript/pull/28558/files#diff-c3ed224e4daa84352f7f1abcd23e8ccaL2273 because it can result in suggestion?
There was a problem hiding this comment.
Is that supposed to be a link to the diff this comment is on? Because it seems to be.
There was a problem hiding this comment.
Ah, naw that position always passes /*isError*/ false, so only manufactures suggestions, anyway. Given that it doesn't check the moduleNotFoundError flag, I assume the intent is to always generate the suggestions.
There was a problem hiding this comment.
So the suggestion is raised only when the container name? Seems flaky then.
There was a problem hiding this comment.
Yeah, it probably is. I assume it was written with the estimation that the module name is resolved for all imports, rather than only-imports-with-used-exports. I didn't add the suggestion, so I'm not sure myself, though.
Fixes #28538
Although I really do think it's strange we don't error on these kinds of imports.