Skip to content
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

Closed
wants to merge 2 commits into from

Conversation

JoostK
Copy link
Member

@JoostK JoostK commented Sep 29, 2019

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

@JoostK JoostK added type: bug/fix action: review The PR is still awaiting reviews from at least one requested reviewer effort1: hours workaround4: none freq1: low severity3: broken target: major This PR is targeted for the next major release risk: low labels Sep 29, 2019
@ngbot ngbot bot added this to the Backlog milestone Sep 29, 2019
@JoostK JoostK marked this pull request as ready for review September 29, 2019 19:36
@JoostK JoostK requested a review from a team as a code owner September 29, 2019 19:36
@petebacondarwin
Copy link
Member

Is there a typo in the commit message?

ngcc used to only a single format,

Copy link
Member

@petebacondarwin petebacondarwin left a 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?

Copy link
Member

@gkalpak gkalpak left a 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 👍

@JoostK
Copy link
Member Author

JoostK commented Sep 30, 2019

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.

@JoostK JoostK added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Sep 30, 2019
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
@JoostK
Copy link
Member Author

JoostK commented Sep 30, 2019

@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.

@JoostK JoostK removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Sep 30, 2019
@JoostK JoostK added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Sep 30, 2019
@atscott atscott closed this in 747f0cf Sep 30, 2019
@naveedahmed1
Copy link
Contributor

I hope this will be added in next release :)

@JoostK
Copy link
Member Author

JoostK commented Oct 2, 2019

@naveedahmed1 It should be available in 9.0.0-next.9 right about now! :D

@naveedahmed1
Copy link
Contributor

Awesome, thank you @JoostK for the update :)

@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes effort1: hours freq1: low risk: low target: major This PR is targeted for the next major release type: bug/fix workaround4: none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants