fix autoimports crash: generate namespace and other module symbol imports #60333
Conversation
| else if (targetSymbol.parent === undefined) { | ||
| Debug.assert(declaration !== undefined, "expected module symbol to have a declaration"); | ||
| importAdder.addImportForModuleSymbol(symbol, isValidTypeOnlyUseSite, declaration); | ||
| } |
There was a problem hiding this comment.
Is there a reason why we can't use isExternalModuleSymbol() as a check?
There was a problem hiding this comment.
Do you mean in the if statement or the debug statement? The debug is needed since the intent of the importAdder function is to copy from an existing declaration
There was a problem hiding this comment.
For the if statement, namespaces are specifically not external module symbols, so isExternalModuleSymbol wouldn't work if trying to export something that was defined as a namespace (React for example)
| @@ -1451,6 +1523,8 @@ export function getImportKind(importingFile: SourceFile | FutureSourceFile, expo | |||
| return getExportEqualsImportKind(importingFile, program.getCompilerOptions(), !!forceImportKeyword); | |||
| case ExportKind.UMD: | |||
| return getUmdImportKind(importingFile, program, !!forceImportKeyword); | |||
| case ExportKind.Module: | |||
| return ImportKind.Namespace; | |||
There was a problem hiding this comment.
I was concerned that there are times this should return ImportKind.CommonJS, but I think the critical cases are already covered by the condition at the top of this function. It’s probably fine.
There was a problem hiding this comment.
I think cases that should return ImportKind.CommonJs still crash. #60333 (comment)
Fixes #59240 (comment) (unfortunately original js case still crashes)
MoveToFile,MoveToNewFile,PasteEdits, and related refactors would crash due to autoimports being unable to resolve where module symbols should be imported from.This PR adds an
importAdderfunction that will generate module imports based off an existing import. The existing import is used as a reference to determine what type of import is generated.Fixes #58740
Fixes #59240 (comment) (unfortunately original js case still crashes)
Fixes #59284
Fixes #59650
Fixes #59651
Fixes #59701
Fixes #59747
#58897 stops crashing, but the resolution isn't quite correct
Closes #59004
Closes #59447
Closes #60302