Store import nodes in SourceFile.imports instead of StringLiteral nodes#22495
Conversation
|
@sheetalkamat @DanielRosenwasser @aozgaa Could someone review this? Underlying issue is marked 2.8.1. |
|
I prefer that we just handle the scenario of getting import declaration from string literal correctly rather than changing the storage from module specifiers to import declarations |
|
No tests? |
|
This was a telemetry issue.. I haven't been able to figure out a case that actually fails at runtime. |
|
@sheetalkamat Please re-review |
|
|
||
| /* @internal */ | ||
| export type AnyValidImportOrReExport = | ||
| | (ImportDeclaration | ExportDeclaration) & { moduleSpecifier: StringLiteral } |
There was a problem hiding this comment.
This seems to be incorrect from https://github.com/Microsoft/TypeScript/pull/22495/files#diff-08a3cc4f1f9a51dbb468c2810f5229d3R1607 where moduleSpecifier is not present ?
| switch (node.parent.kind) { | ||
| case SyntaxKind.ImportDeclaration: | ||
| case SyntaxKind.ExportDeclaration: | ||
| return node.parent as AnyValidImportOrReExport; |
There was a problem hiding this comment.
And this is where we might crash as moduleSpecifier is absent on import declaration at: https://github.com/Microsoft/TypeScript/pull/22495/files#diff-08a3cc4f1f9a51dbb468c2810f5229d3R1607 ?
| imports = append(imports, node.arguments[0]); | ||
| } | ||
| // we have to check the argument list has length of 1. We will still have to process these even though we have parsing error. | ||
| else if (isImportCall(node) && node.arguments.length === 1 && node.arguments[0].kind === SyntaxKind.StringLiteral) { |
There was a problem hiding this comment.
Do we need to check instead isStringLiteralLike(node.arguments[0]) instead of checking just string literal ?
There was a problem hiding this comment.
isRequireCall checks for StringLiteralLike, updated the parameter name
There was a problem hiding this comment.
No i meant
// we have to check the argument list has length of 1. We will still have to process these even though we have parsing error.
else if (isImportCall(node) && node.arguments.length === 1 && node.arguments[0].kind === SyntaxKind.StringLiteral) {There was a problem hiding this comment.
Oh, sure. Now we can get better typings for:
const foo = await import(`foo`);
Fixes #20562
Previously
SourceFile.importswas a list ofStringLiterals and we had to climb parents to get to the importer node, which is error-prone asnode.parentis just aNode. Now we store the import nodes directly and climb down if we need theStringLiteral.