Improve error handling for custom/duplicate decorators #54139

pmvald · 2024-01-29T02:20:24Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/main/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

Yes
No

Other information

pmvald · 2024-01-29T05:20:04Z

TGP link

MeftunH · 2024-01-29T08:54:04Z

packages/compiler-cli/src/ngtsc/transform/src/compilation.ts

+  if (isCore) {
+    return decorator.name === 'Injectable';
+  } else if (decorator.import !== null && decorator.import.from === '@angular/core') {
+    return decorator.import.name === 'Injectable';
+  }


I think this is unclear usage. So, it makes harder to find changes for next versions

I'm not sure I follow. Can you elaborate please?

MeftunH · 2024-01-29T08:54:31Z

packages/compiler-cli/src/ngtsc/core/src/compiler.ts

+        semanticDepGraphUpdater, this.adapter, isCore, !!this.options.forbidOrphanComponents);



why we did use magic operator in here?

devversion · 2024-01-29T11:41:38Z

packages/compiler-cli/src/ngtsc/transform/src/compilation.ts

+    // 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 unmatchedDecoratorsInLocalMode =


Suggested change

const unmatchedDecoratorsInLocalMode =

const unmatchedDecorators =

Regarding "fixing deps tracker". There is no fix we are landing right? except hoping for ES decorators at some point. The comment sound like deps tracker needs a fix.

Done!

It is possible to fix this issue. It requires some emit shenanigans to somehow connect a decorated class to its pre-decorated version. I have a solution in mind. We can do this if the number of use cases increases. Right now I'm aware of one app using custom decorators for components (and even that one marked the decorator as deprecated as plan to remove).

I was just curious because of the comment sounding like we intend to allow this in the future again. Maybe remove?

devversion · 2024-01-29T11:42:50Z

packages/compiler-cli/src/ngtsc/transform/src/compilation.ts

+  if (isCore) {
+    return decorator.name === 'Injectable';
+  } else if (decorator.import !== null && decorator.import.from === '@angular/core') {
+    return decorator.import.name === 'Injectable';
+  }


I'm not sure I follow. Can you elaborate please?

packages/compiler-cli/src/ngtsc/transform/src/compilation.ts

packages/compiler-cli/src/ngtsc/transform/test/compilation_spec.ts

…in local compilation diagnostics For cases like this: ``` @component({...}) @component({...}) export class SomeComp { } ``` The `DecoratorHandler.detect` apparantly matches only one of the `@Component` decorator, leaving the other undetected which will be transformed by TS decorator helper and that breaks local compilation runtimes. But the error message only mentioned "custom" decorator, while in this case it is a "duplicate Angular" decorator. The respective error message is updated thus.

devversion

LGTM, please add merge ready once you think it's ready

packages/compiler-cli/src/ngtsc/transform/src/compilation.ts

…classes in local compilation mode Custom/duplicate decorators break the deps tracker in local mode. But deps tracker only deals with non-injectable classes. So applying custom/duplicate decorators to `@Injectable` only classes does not disturb deps tracker and local compilation in general. There are also ~ 100 such cases in g3 which cannot be cleaned up.

…bidOrphanComponents` 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 as orphan if it or its NgModule have custom/duplicate decorators. So it is unsafe to use this option in the presence of custom/duplicate decorator and we disable it until it is made compatible. Note that applying custom/duplicate decorators to `@Injectable` classes is ok since these classes never make it into the deps tracker. So we excempt them.

…classes in local compilation mode (#54139) Custom/duplicate decorators break the deps tracker in local mode. But deps tracker only deals with non-injectable classes. So applying custom/duplicate decorators to `@Injectable` only classes does not disturb deps tracker and local compilation in general. There are also ~ 100 such cases in g3 which cannot be cleaned up. PR Close #54139

…bidOrphanComponents` is set (#54139) 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 as orphan if it or its NgModule have custom/duplicate decorators. So it is unsafe to use this option in the presence of custom/duplicate decorator and we disable it until it is made compatible. Note that applying custom/duplicate decorators to `@Injectable` classes is ok since these classes never make it into the deps tracker. So we excempt them. PR Close #54139

jessicajaniuk · 2024-02-05T15:07:45Z

This PR was merged into the repository by commit 96bcf4f.

…ion `forbidOrphanComponents` is set (angular#54139)" This reverts commit 96bcf4f.

…ectable

…ectable classes in local compilation mode (angular#54139)" This reverts commit a592904.

…orators in local compilation diagnostics (angular#54139)" This reverts commit 4b1d948.

…ion `forbidOrphanComponents` is set (#54139)" (#54264) This reverts commit 96bcf4f. PR Close #54264

…ectable

…ectable classes in local compilation mode (#54139)" (#54264) This reverts commit a592904. PR Close #54264

…orators in local compilation diagnostics (#54139)" (#54264) This reverts commit 4b1d948. PR Close #54264

jessicajaniuk · 2024-02-05T18:19:24Z

@pmvald This caused a breakage in G3 and has been reverted.

JeanMeche · 2024-11-26T20:32:27Z

This PR has gone stale.

angular-automatic-lock-bot · 2024-12-27T00:11:45Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

pmvald added action: review area: compiler target: minor PullApprove: disable labels Jan 29, 2024

pmvald requested a review from devversion January 29, 2024 02:20

ngbot bot modified the milestone: Backlog Jan 29, 2024

pmvald force-pushed the orphan-comps branch 3 times, most recently from 90cb601 to 1f3280a Compare January 29, 2024 05:06

pmvald changed the title ~~fix(compiler-cli): forbid custom decorator when option `forbidOrphanC…~~ Improve error handling for custom/duplicate decorators Jan 29, 2024

pmvald force-pushed the orphan-comps branch from 1f3280a to b19175e Compare January 29, 2024 05:28

MeftunH reviewed Jan 29, 2024

View reviewed changes

devversion reviewed Jan 29, 2024

View reviewed changes

pmvald force-pushed the orphan-comps branch from b19175e to 32be7b6 Compare January 30, 2024 02:17

pmvald force-pushed the orphan-comps branch from 32be7b6 to 8d8d046 Compare January 30, 2024 18:13

devversion approved these changes Feb 1, 2024

View reviewed changes

packages/compiler-cli/src/ngtsc/transform/src/compilation.ts Show resolved Hide resolved

packages/compiler-cli/src/ngtsc/transform/src/compilation.ts Outdated Show resolved Hide resolved

packages/compiler-cli/src/ngtsc/transform/src/compilation.ts Show resolved Hide resolved

pmvald force-pushed the orphan-comps branch from 8d8d046 to 971b730 Compare February 3, 2024 03:41

pmvald added 2 commits February 2, 2024 22:47

pmvald force-pushed the orphan-comps branch from 971b730 to aa9efe3 Compare February 3, 2024 03:47

pmvald added action: merge and removed action: review labels Feb 5, 2024

jessicajaniuk closed this in 4b1d948 Feb 5, 2024

jessicajaniuk mentioned this pull request Feb 5, 2024

Revert "fix(compiler-cli): forbid custom/duplicate decorator when opt… #54264

Closed

jessicajaniuk pushed a commit to jessicajaniuk/angular that referenced this pull request Feb 5, 2024

Revert "fix(compiler-cli): allow custom/duplicate decorators for @Inj…

b5e3292

…ectable classes in local compilation mode (angular#54139)" This reverts commit a592904.

jessicajaniuk pushed a commit that referenced this pull request Feb 5, 2024

Revert "fix(compiler-cli): forbid custom/duplicate decorator when opt…

a2af706

…ion `forbidOrphanComponents` is set (#54139)" (#54264) This reverts commit 96bcf4f. PR Close #54264

jessicajaniuk pushed a commit that referenced this pull request Feb 5, 2024

Revert "fix(compiler-cli): allow custom/duplicate decorators for @Inj…

bd43aa3

…ectable classes in local compilation mode (#54139)" (#54264) This reverts commit a592904. PR Close #54264

jessicajaniuk reopened this Feb 5, 2024

jessicajaniuk added state: blocked action: global presubmit and removed action: merge PullApprove: disable labels Feb 5, 2024

JeanMeche closed this Nov 26, 2024

angular-automatic-lock-bot bot locked and limited conversation to collaborators Dec 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve error handling for custom/duplicate decorators #54139

Improve error handling for custom/duplicate decorators #54139

pmvald commented Jan 29, 2024 •

edited

Loading

pmvald commented Jan 29, 2024 •

edited

Loading

MeftunH Jan 29, 2024

devversion Jan 29, 2024

MeftunH Jan 29, 2024

devversion Jan 29, 2024

pmvald Jan 30, 2024

devversion Feb 1, 2024

devversion Jan 29, 2024

devversion left a comment

jessicajaniuk commented Feb 5, 2024

jessicajaniuk commented Feb 5, 2024

JeanMeche commented Nov 26, 2024

angular-automatic-lock-bot bot commented Dec 27, 2024

		semanticDepGraphUpdater, this.adapter, isCore, !!this.options.forbidOrphanComponents);

	const unmatchedDecoratorsInLocalMode =
	const unmatchedDecorators =

Improve error handling for custom/duplicate decorators #54139

Improve error handling for custom/duplicate decorators #54139

Conversation

pmvald commented Jan 29, 2024 • edited Loading

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

pmvald commented Jan 29, 2024 • edited Loading

MeftunH Jan 29, 2024

Choose a reason for hiding this comment

devversion Jan 29, 2024

Choose a reason for hiding this comment

MeftunH Jan 29, 2024

Choose a reason for hiding this comment

devversion Jan 29, 2024

Choose a reason for hiding this comment

pmvald Jan 30, 2024

Choose a reason for hiding this comment

devversion Feb 1, 2024

Choose a reason for hiding this comment

devversion Jan 29, 2024

Choose a reason for hiding this comment

devversion left a comment

Choose a reason for hiding this comment

jessicajaniuk commented Feb 5, 2024

jessicajaniuk commented Feb 5, 2024

JeanMeche commented Nov 26, 2024

angular-automatic-lock-bot bot commented Dec 27, 2024

pmvald commented Jan 29, 2024 •

edited

Loading

pmvald commented Jan 29, 2024 •

edited

Loading