Report errors correctly in watch mode by invalidating errors from files referencing modules that export from changed file #25593
Conversation
…ostics to be calculated
src/compiler/builder.ts
Outdated
| /** | ||
| * True if the semantic diagnostics were copied from the old state | ||
| */ | ||
| semanticDiagnosticsFromOldStateFiles?: number; |
There was a problem hiding this comment.
This number isn't used except as a counting semaphore (it only increments and decrements) and it has a lot of debug asserts around that - this leads me to believe it should more naturally be a Map<true> instead. (And then truthiness checks would be on semanticDiagnosticsFromOldStateFiles.size instead) Although changing the type of semanticDiagnosticsPerFile from Map<ReadonlyArray<Diagnostic>> to Map<{ fromOldState: boolean, diagnostics: ReadonlyArray<Diagnostic> }> could be even better and make even more of the sanity checking assertions unnecessary.
src/compiler/types.ts
Outdated
| @@ -2624,6 +2624,14 @@ namespace ts { | |||
| /* @internal */ pragmas: PragmaMap; | |||
| /* @internal */ localJsxNamespace?: __String; | |||
| /* @internal */ localJsxFactory?: EntityName; | |||
|
|
|||
| /*@internal*/ getExportedModulesFromDeclarationEmit?(): ExportedModulesFromDeclarationEmit; | |||
There was a problem hiding this comment.
This doesn't need to be a function/thunk - the work is already done by the time the method is present - just attach the lists to the file directly.
src/compiler/types.ts
Outdated
|
|
||
| /*@internal*/ | ||
| export interface ExportedModulesFromDeclarationEmit { | ||
| exportedModuleSpecifiers: ReadonlyArray<StringLiteralLike>; |
There was a problem hiding this comment.
From what I can tell, both of these just get mapped to a path eventually, via lookup (string ->) symbol -> sourcefile -> path. Rather than storing both symbols and string literals here, I'd rather we just stored the paths for all kinds directly. Minimally, just one field with symbols for both (from which path can be easily found) - having two fields for different levels of processing of essentially the same thing seems somewhat extraneous. This may necessitate exposing something checker-related on the EmitResolver.
There was a problem hiding this comment.
I think making it list of Symbols makes sense. ( though they get converted to paths, I would rather do that work in builder since it is shared with how we handle the imports in the module)
…ombining exportSpecifiers emitted and dynamic import type nodes written
|
@weswigham can you please take a look again. |
Fixes #24986
The idea here is that when we calculate the declaration emit output for the file to see if it is valid, we also store the module specifiers and import type nodes written that are needed as the exported members are referenced from those modules.
The first iteration would not have valid result but we would anyways have all files as changed in that, But during second iteration if file say c.ts is changed, we look for files that are exporting c.ts and then look for the files that reference these modules. So in test scenario of #24986, when c.ts is changed => we find that b.ts has exported module c.ts so any file that references b.ts need to invalidate the semantic diagnostics. (note we don't need to emit that file, just the diagnostics since that's the only change.)
Better approach instead of #25534