-
Notifications
You must be signed in to change notification settings - Fork 25.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(ngcc): handle presence of both ctorParameters
and __decorate
#32901
Conversation
Is there a typo in the commit message?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - I wonder if there is a risk that tools like ng-packagr may mix such decorator styles even more loosely? In which case, should we just change the code to look for both types of decorator always and then merge the two sets that are discovered? This would make use resilient to such tooling changes in the future. The only downside would be a small performance loss, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for always looking for both types of decorators and then merging the two sets (unless the performance decrease is significant).
Otherwise lgtm2 👍
I thought about merging the two and decided not to, as it would require some merging strategy that may depend yet again on how tooling generates code. However I see now that before the refactoring in #31614 we were essentially already merging the two formats, so it makes sense to start doing that again. |
Recently ng-packagr was updated to include a transform that used to be done in tsickle (ng-packagr/ng-packagr#1401), where only constructor parameter decorators are emitted in tsickle's format, not any of the other decorators. ngcc used to extract decorators from only a single format, so once it saw the `ctorParameters` static property it assumed the library is using the tsickle format. Therefore, none of the `__decorate` calls were considered. This resulted in missing decorator information, preventing proper processing of a package. This commit changes how decorators are extracted by always looking at both the static properties and the `__decorate` calls, merging these sources appropriately. Resolves FW-1573
e281f90
to
9d35d51
Compare
@petebacondarwin I fixed the commit message and pushed a fixup commit so that the two decorator sources are now merged, preferring static properties. Regarding performance, I don't expect it to matter that much. Recall that for non-tsickle libraries we were already executing both code paths. |
9d35d51
to
887e57b
Compare
I hope this will be added in next release :) |
@naveedahmed1 It should be available in |
Awesome, thank you @JoostK for the update :) |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Recently ng-packagr was updated to include a transform that used to be
done in tsickle (ng-packagr/ng-packagr#1401),
where only constructor parameter decorators are emitted in tsickle's
format, not any of the other decorators.
ngcc used to extract decorators from only a single format, so once it
saw the
ctorParameters
static property it assumed the library is usingthe tsickle format. Therefore, none of the
__decorate
calls wereconsidered. This resulted in missing decorator information, preventing
proper processing of a package.
This commit changes how decorators are extracted by always looking at
both the static properties and the
__decorate
calls, merging thesesources appropriately.
Resolves FW-1573