Skip to content

Commit

Permalink
fix(compiler-cli): forbid custom decorator when option `forbidOrphanC…
Browse files Browse the repository at this point in the history
…omponents` is set

The deps tracker which is responsible to track orphan components does not work for classes mutated by custom decorator. Some work needed to make this happen (tracked in b/320536434). As a result with option `forbidOrphanComponents` being true the deps tracker will falsely report any component orphan if it or ots NgModule have custom decorators. So it is unsafe to use this option in the presence of custom decorator, and so we disable it until it is made compatible.
  • Loading branch information
pmvald committed Jan 29, 2024
1 parent c213a4e commit 90cb601
Show file tree
Hide file tree
Showing 4 changed files with 74 additions and 27 deletions.
2 changes: 1 addition & 1 deletion packages/compiler-cli/src/ngtsc/core/src/compiler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1167,7 +1167,7 @@ export class NgCompiler {
const traitCompiler = new TraitCompiler(
handlers, reflector, this.delegatingPerfRecorder, this.incrementalCompilation,
this.options.compileNonExportedClasses !== false, compilationMode, dtsTransforms,
semanticDepGraphUpdater, this.adapter);
semanticDepGraphUpdater, this.adapter, !!this.options.forbidOrphanComponents);

// Template type-checking may use the `ProgramDriver` to produce new `ts.Program`(s). If this
// happens, they need to be tracked by the `NgCompiler`.
Expand Down
71 changes: 46 additions & 25 deletions packages/compiler-cli/src/ngtsc/transform/src/compilation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,15 +97,13 @@ export class TraitCompiler implements ProgramTypeCheckAdapter {

constructor(
private handlers: DecoratorHandler<unknown, unknown, SemanticSymbol|null, unknown>[],
private reflector: ReflectionHost,
private perf: PerfRecorder,
private reflector: ReflectionHost, private perf: PerfRecorder,
private incrementalBuild: IncrementalBuild<ClassRecord, unknown>,
private compileNonExportedClasses: boolean,
private compilationMode: CompilationMode,
private compileNonExportedClasses: boolean, private compilationMode: CompilationMode,
private dtsTransforms: DtsTransformRegistry,
private semanticDepGraphUpdater: SemanticDepGraphUpdater|null,
private sourceFileTypeIdentifier: SourceFileTypeIdentifier,
) {
private readonly forbidOrphanComponents: boolean) {
for (const handler of handlers) {
this.handlersByName.set(handler.name, handler);
}
Expand Down Expand Up @@ -259,19 +257,25 @@ export class TraitCompiler implements ProgramTypeCheckAdapter {
let record: ClassRecord|null = this.recordFor(clazz);
let foundTraits: PendingTrait<unknown, unknown, SemanticSymbol|null, unknown>[] = [];

// A set to track the non-Angular decorators in local compilation mode. An error will be issued
// if non-Angular decorators is found in local compilation mode.
const nonNgDecoratorsInLocalMode =
this.compilationMode === CompilationMode.LOCAL ? new Set(decorators) : null;
// A set to track and forbid non-Angular decorators. Such ban is needed in some cases where
// custom decorators are not supported (yet). Current cases are: 1) local compilation mode, 2)
// when option `forbidOrphanComponents` is set. These two cases share the same solution as the
// reason for both cases is that the `DepsTracker` is not able to understand components mutated
// by decorators. Once the `DepsTracker` is fixed then these issues will be resolved. This issue
// is to be fixed soon (b/320536434).
const nonNgDecorators =
(this.compilationMode === CompilationMode.LOCAL || this.forbidOrphanComponents) ?
new Set(decorators) :
null;

for (const handler of this.handlers) {
const result = handler.detect(clazz, decorators);
if (result === undefined) {
continue;
}

if (nonNgDecoratorsInLocalMode !== null && result.decorator !== null) {
nonNgDecoratorsInLocalMode.delete(result.decorator);
if (nonNgDecorators !== null && result.decorator !== null) {
nonNgDecorators.delete(result.decorator);
}

const isPrimaryHandler = handler.precedence === HandlerPrecedence.PRIMARY;
Expand Down Expand Up @@ -341,20 +345,37 @@ export class TraitCompiler implements ProgramTypeCheckAdapter {
}
}

if (nonNgDecoratorsInLocalMode !== null && nonNgDecoratorsInLocalMode.size > 0 &&
record !== null && record.metaDiagnostics === null) {
// Custom decorators found in local compilation mode! In this mode we don't support custom
// decorators yet. But will eventually do (b/320536434). For now a temporary error is thrown.
record.metaDiagnostics = [...nonNgDecoratorsInLocalMode].map(
decorator => ({
category: ts.DiagnosticCategory.Error,
code: Number('-99' + ErrorCode.DECORATOR_UNEXPECTED),
file: getSourceFile(clazz),
start: decorator.node.getStart(),
length: decorator.node.getWidth(),
messageText:
'In local compilation mode, Angular does not support custom decorators. Ensure all class decorators are from Angular.',
}));
if (nonNgDecorators !== null && nonNgDecorators.size > 0 && record !== null &&
record.metaDiagnostics === null) {
if (this.compilationMode === CompilationMode.LOCAL) {
// Custom decorators found in local compilation mode! In this mode we don't support custom
// decorators yet. But will eventually do (b/320536434). For now a temporary error is
// thrown.
record.metaDiagnostics = [...nonNgDecorators].map(
decorator => ({
category: ts.DiagnosticCategory.Error,
code: Number('-99' + ErrorCode.DECORATOR_UNEXPECTED),
file: getSourceFile(clazz),
start: decorator.node.getStart(),
length: decorator.node.getWidth(),
messageText:
'In local compilation mode, Angular does not support custom decorators. Ensure all class decorators are from Angular.',
}));
} else if (this.forbidOrphanComponents) {
// Custom decorators found when option `forbidOrphanComponents` is set! for this option we
// don't support custom decorators yet. But will eventually do (b/320536434). For now a
// temporary error is thrown.
record.metaDiagnostics = [...nonNgDecorators].map(
decorator => ({
category: ts.DiagnosticCategory.Error,
code: Number('-99' + ErrorCode.DECORATOR_UNEXPECTED),
file: getSourceFile(clazz),
start: decorator.node.getStart(),
length: decorator.node.getWidth(),
messageText:
'When the Angular compiler option "forbidOrphanComponents" is set, Angular does not support custom decorators. Ensure all class decorators are from Angular.',
}));
}
record.traits = foundTraits = [];
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ runInEachFileSystem(() => {
const reflectionHost = new TypeScriptReflectionHost(checker);
const compiler = new TraitCompiler(
handlers, reflectionHost, NOOP_PERF_RECORDER, NOOP_INCREMENTAL_BUILD, true,
compilationMode, new DtsTransformRegistry(), null, fakeSfTypeIdentifier);
compilationMode, new DtsTransformRegistry(), null, fakeSfTypeIdentifier, false);
const sourceFile = program.getSourceFile(filename)!;

return {compiler, sourceFile, program, filename: _('/' + filename)};
Expand Down
26 changes: 26 additions & 0 deletions packages/compiler-cli/test/ngtsc/ngtsc_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -552,6 +552,32 @@ function allTests(os: string) {
})]);
});

it('should error for custom decorators when forbidOrphanComponents is true', () => {
env.tsconfig({
forbidOrphanComponents: true,
});
env.write('test.ts', `
import {Component} from '@angular/core';
function customDecorator<T extends new (...args: any[]) => {}>(original: T) {
return class extends original {
someProp = 'default';
};
}
@Component({template: ''})
@customDecorator
class MyComp {}
`);

const diagnostics = env.driveDiagnostics();

expect(diagnostics).toEqual([jasmine.objectContaining({
messageText: jasmine.stringMatching(
/When the Angular compiler option "forbidOrphanComponents" is set, Angular does not support custom decorators/),
})]);
});

// This test triggers the Tsickle compiler which asserts that the file-paths
// are valid for the real OS. When on non-Windows systems it doesn't like paths
// that start with `C:`.
Expand Down

0 comments on commit 90cb601

Please sign in to comment.