Skip to content

Commit

Permalink
fix(compiler-cli): Show template syntax errors in local compilation m…
Browse files Browse the repository at this point in the history
…odified (#55855)

Currently the template syntax errors are extracted in the template type check phase. But in local compilation mode we skip the type check phase. As a result template syntax errors are not displayed. With this change we show the template syntax diagnostics in local mode.

PR Close #55855
  • Loading branch information
pmvald authored and thePunderWoman committed May 31, 2024
1 parent 9d034f6 commit 9e21582
Show file tree
Hide file tree
Showing 5 changed files with 111 additions and 34 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ ts_library(
"//packages/compiler-cli/src/ngtsc/reflection",
"//packages/compiler-cli/src/ngtsc/scope",
"//packages/compiler-cli/src/ngtsc/transform",
"//packages/compiler-cli/src/ngtsc/typecheck",
"//packages/compiler-cli/src/ngtsc/typecheck/api",
"//packages/compiler-cli/src/ngtsc/typecheck/extended/api",
"//packages/compiler-cli/src/ngtsc/typecheck/template_semantics/api",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ import {
HandlerPrecedence,
ResolveResult,
} from '../../../transform';
import {TypeCheckableDirectiveMeta, TypeCheckContext} from '../../../typecheck/api';
import {TemplateId, TypeCheckableDirectiveMeta, TypeCheckContext} from '../../../typecheck/api';
import {ExtendedTemplateChecker} from '../../../typecheck/extended/api';
import {TemplateSemanticsChecker} from '../../../typecheck/template_semantics/api/api';
import {getSourceFile} from '../../../util/src/typescript';
Expand Down Expand Up @@ -176,6 +176,7 @@ import {
collectAnimationNames,
validateAndFlattenComponentImports,
} from './util';
import {getTemplateDiagnostics} from '../../../typecheck';

const EMPTY_ARRAY: any[] = [];

Expand Down Expand Up @@ -624,6 +625,25 @@ export class ComponentDecoratorHandler
},
this.compilationMode,
);

if (
this.compilationMode === CompilationMode.LOCAL &&
template.errors &&
template.errors.length > 0
) {
// Template errors are handled at the type check phase. But we skip this phase in local compilation mode. As a result we need to handle the errors now and add them to the diagnostics.
if (diagnostics === undefined) {
diagnostics = [];
}

diagnostics.push(
...getTemplateDiagnostics(
template.errors,
'' as TemplateId, // Template ID is required as part of the template type check, mainly for mapping the template to its component class. But here we are generating the diagnostic outside of the type check context, and so we skip the template ID.
template.sourceMapping,
),
);
}
}
const templateResource = template.declaration.isInline
? {path: null, expression: component.get('template')!}
Expand Down Expand Up @@ -1578,10 +1598,6 @@ export class ComponentDecoratorHandler
resolution: Readonly<Partial<ComponentResolutionData>>,
pool: ConstantPool,
): CompileResult[] {
if (analysis.template.errors !== null && analysis.template.errors.length > 0) {
return [];
}

// In the local compilation mode we can only rely on the information available
// within the `@Component.deferredImports` array, because in this mode compiler
// doesn't have information on which dependencies belong to which defer blocks.
Expand Down
2 changes: 1 addition & 1 deletion packages/compiler-cli/src/ngtsc/typecheck/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,6 @@
*/

export {FileTypeCheckingData, TemplateTypeCheckerImpl} from './src/checker';
export {TypeCheckContextImpl} from './src/context';
export {TypeCheckContextImpl, getTemplateDiagnostics} from './src/context';
export {TypeCheckShimGenerator} from './src/shim';
export {typeCheckFilePath} from './src/type_check_file';
54 changes: 26 additions & 28 deletions packages/compiler-cli/src/ngtsc/typecheck/src/context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -250,9 +250,7 @@ export class TypeCheckContextImpl implements TypeCheckContext {
const templateDiagnostics: TemplateDiagnostic[] = [];

if (parseErrors !== null) {
templateDiagnostics.push(
...this.getTemplateDiagnostics(parseErrors, templateId, sourceMapping),
);
templateDiagnostics.push(...getTemplateDiagnostics(parseErrors, templateId, sourceMapping));
}

const boundTarget = binder.bind({template});
Expand Down Expand Up @@ -559,33 +557,33 @@ export class TypeCheckContextImpl implements TypeCheckContext {

return this.fileMap.get(sfPath)!;
}
}

private getTemplateDiagnostics(
parseErrors: ParseError[],
templateId: TemplateId,
sourceMapping: TemplateSourceMapping,
): TemplateDiagnostic[] {
return parseErrors.map((error) => {
const span = error.span;

if (span.start.offset === span.end.offset) {
// Template errors can contain zero-length spans, if the error occurs at a single point.
// However, TypeScript does not handle displaying a zero-length diagnostic very well, so
// increase the ending offset by 1 for such errors, to ensure the position is shown in the
// diagnostic.
span.end.offset++;
}
export function getTemplateDiagnostics(
parseErrors: ParseError[],
templateId: TemplateId,
sourceMapping: TemplateSourceMapping,
): TemplateDiagnostic[] {
return parseErrors.map((error) => {
const span = error.span;

if (span.start.offset === span.end.offset) {
// Template errors can contain zero-length spans, if the error occurs at a single point.
// However, TypeScript does not handle displaying a zero-length diagnostic very well, so
// increase the ending offset by 1 for such errors, to ensure the position is shown in the
// diagnostic.
span.end.offset++;
}

return makeTemplateDiagnostic(
templateId,
sourceMapping,
span,
ts.DiagnosticCategory.Error,
ngErrorCode(ErrorCode.TEMPLATE_PARSE_ERROR),
error.msg,
);
});
}
return makeTemplateDiagnostic(
templateId,
sourceMapping,
span,
ts.DiagnosticCategory.Error,
ngErrorCode(ErrorCode.TEMPLATE_PARSE_ERROR),
error.msg,
);
});
}

/**
Expand Down
62 changes: 62 additions & 0 deletions packages/compiler-cli/test/ngtsc/local_compilation_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2623,5 +2623,67 @@ runInEachFileSystem(() => {
);
});
});

describe('template diagnostics', () => {
it('should show correct error message for syntatic template errors - case of inline template', () => {
env.write(
'test.ts',
`
import {Component} from '@angular/core';
@Component({
template: '<span Hello! </span>',
})
export class Main {
}
`,
);

const errors = env.driveDiagnostics();

expect(errors.length).toBeGreaterThanOrEqual(1);

const {code, messageText} = errors[0];

expect(code).toBe(ngErrorCode(ErrorCode.TEMPLATE_PARSE_ERROR));

const text = ts.flattenDiagnosticMessageText(messageText, '\n');

expect(text).toContain('Opening tag "span" not terminated');
});

it('should show correct error message for syntatic template errors - case of external template', () => {
env.write(
'test.ts',
`
import {Component} from '@angular/core';
@Component({
templateUrl: 'test.ng.html',
})
export class Main {
}
`,
);
env.write(
'test.ng.html',
`
<span Hello! </span>
`,
);

const errors = env.driveDiagnostics();

expect(errors.length).toBeGreaterThanOrEqual(1);

const {code, messageText} = errors[0];

expect(code).toBe(ngErrorCode(ErrorCode.TEMPLATE_PARSE_ERROR));

const text = ts.flattenDiagnosticMessageText(messageText, '\n');

expect(text).toContain('Opening tag "span" not terminated');
});
});
});
});

0 comments on commit 9e21582

Please sign in to comment.