refactor(compiler): show meaningful diagnostic when reporting a template error fails #44001
Conversation
Thanks for doing this Joost!
| parseTemplateAsSourceFileForTest = null; | ||
| } | ||
|
|
||
| function parseTemplateAsSourceFile(fileName: string, template: string): ts.SourceFile { |
There was a problem hiding this comment.
Instead of this fake function with set*() and reset*() functions, could we just use Jasmine spies for that? It's effectively the same thing, but the test framework handles all the setup and restore functionality.
There was a problem hiding this comment.
I am not sure how to achieve that nicely in this case, TBH. Spying on ts.createSourceFile is problematic in the sense that the test calls into that function multiple times, so we'd have to conditionally call through which I don't like. As for parseTemplateAsSourceFile, even if it becomes exported will Jasmine be able to spy on the ES export? That sounds sketchy to me.
There was a problem hiding this comment.
Spying on an internal function is a bit awkward, but set*() and reset*() are already exported to work around this, so I'm not sure it's that much worse.
If ts.createSourceFile() is too noisy, then we could make parseTemplateAsSourceFile() just call through:
export function parseTemplateAsSourceFile(fileName: string, template: string): ts.SourceFile {
return ts.createSourceFile(fileName, template, ts.ScriptTarget.Latest, false, ts.ScriptKind.JSX);
}We could put this in another file on its own so as not to pollute the public API of diagnostic.ts and then it can be spied on in the test.
I haven't tried Jasmine spies with real ES modules, so I'm not sure if there are caveats to it. You may need to import * as foo from '...'; so the spy will pick up the call. I still think that's an improvement over putting multiple test-only functions into the prod build and inventing our own mocking semantics for this file.
// diagnostic.ts
import * as templateParser from './parse_template';
export function makeTemplateDiagnostic(/* ... */): TemplateDiagnostic {
// ...
try {
templateParser.parseTemplateAsSourceFile(/* ... */);
} catch (err) {
// ...
}
}// test.ts
import * as templateParser from './parse_template';
it('test', () => {
spyOn(templateParser, 'parseTemplateAsSourceFile').and.returnValue(/* ... */);
// ...
});
| parseTemplateAsSourceFileForTest = null; | ||
| } | ||
|
|
||
| function parseTemplateAsSourceFile(fileName: string, template: string): ts.SourceFile { |
There was a problem hiding this comment.
Spying on an internal function is a bit awkward, but set*() and reset*() are already exported to work around this, so I'm not sure it's that much worse.
If ts.createSourceFile() is too noisy, then we could make parseTemplateAsSourceFile() just call through:
export function parseTemplateAsSourceFile(fileName: string, template: string): ts.SourceFile {
return ts.createSourceFile(fileName, template, ts.ScriptTarget.Latest, false, ts.ScriptKind.JSX);
}We could put this in another file on its own so as not to pollute the public API of diagnostic.ts and then it can be spied on in the test.
I haven't tried Jasmine spies with real ES modules, so I'm not sure if there are caveats to it. You may need to import * as foo from '...'; so the spy will pick up the call. I still think that's an improvement over putting multiple test-only functions into the prod build and inventing our own mocking semantics for this file.
// diagnostic.ts
import * as templateParser from './parse_template';
export function makeTemplateDiagnostic(/* ... */): TemplateDiagnostic {
// ...
try {
templateParser.parseTemplateAsSourceFile(/* ... */);
} catch (err) {
// ...
}
}// test.ts
import * as templateParser from './parse_template';
it('test', () => {
spyOn(templateParser, 'parseTemplateAsSourceFile').and.returnValue(/* ... */);
// ...
});…ate error fails When the Angular compiler emits a diagnostic in a template file, it forces TypeScript to parse that template. Templates are not TypeScript, so this parse finds a bunch of parsing errors, which Angular then ignores and we show the diagnostic anyways because we have more context. This can lead to strange behavior in TypeScript because templates are so weird that it can break the parser and crash the whole compiler. For example, certain Angular templates can encounter failures fixed by microsoft/TypeScript#45987, which are not easily debuggable and require a TS upgrade to fix. This commit introduces logic to handle the error gracefully, by falling back to report the template error on the component class itself. The diagnostic is extended to still reference the template location and includes the failure's stack trace, to allow the parsing failure to be reported to TypeScript (as parsing should in theory not cause a crash). Closes angular#43970
…a template error fails
…a template error fails
When the Angular compiler emits a diagnostic in a template file, it
forces TypeScript to parse that template. Templates are not TypeScript,
so this parse finds a bunch of parsing errors, which Angular then
ignores and we show the diagnostic anyways because we have more context.
This can lead to strange behavior in TypeScript because templates are so
weird that it can break the parser and crash the whole compiler.
For example, certain Angular templates can encounter failures fixed by
microsoft/TypeScript#45987, which are not easily debuggable and require
a TS upgrade to fix.
This commit introduces logic to handle the error gracefully, by falling
back to report the template error on the component class itself. The
diagnostic is extended to still reference the template location and
includes the failure's stack trace, to allow the parsing failure to be
reported to TypeScript (as parsing should in theory not cause a crash).
Closes #43970
@dgp1130 PTAL, curious to hear if you think this is sufficient or if you have suggestions to improve it.
The text was updated successfully, but these errors were encountered: