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.

Sign up for GitHub

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

Jump to bottom

fix(ivy): proper resolution of Enums in Component decorator #27971

Closed
wants to merge 4 commits into from
Closed

fix(ivy): proper resolution of Enums in Component decorator #27971

wants to merge 4 commits into from

Conversation

AndrewKushnir
Copy link
Contributor

Prior to this change Component decorator was resolving encapsulation value a bit incorrectly, which resulted in encapsulation: NaN in compiled code. Now we resolve the value as Enum member and throw if it's not the case. As a part of this update, the changeDetection field handling is also added, the resolution logic is the same as the one used for encapsulation field.

This PR resolves issue FW-722.

PR Type

What kind of change does this PR introduce?

  • Bugfix

Does this PR introduce a breaking change?

  • Yes
  • No

@AndrewKushnir AndrewKushnir added type: bug/fix action: review The PR is still awaiting reviews from at least one requested reviewer target: major This PR is targeted for the next major release comp: ivy labels Jan 8, 2019
@AndrewKushnir AndrewKushnir requested a review from a team as a code owner January 8, 2019 00:42
@ngbot ngbot bot modified the milestone: needsTriage Jan 8, 2019
@mary-poppins
Copy link

You can preview fe6688d at https://pr27971-fe6688d.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 561dea7 at https://pr27971-561dea7.ngbuilds.io/.

@AndrewKushnir
fix(ivy): proper resolution of Enums in Component decorator
29e6423 
Prior to this change Component decorator was resolving `encapsulation` value a bit incorrectly, which resulted in `encapsulation: NaN` in compiled code. Now we resolve the value as Enum memeber and throw if it's not the case. As a part of this update, the `changeDetection` field handling is also added, the resolution logic is the same as the one used for `encapsulation` field.
@AndrewKushnir
fixup! fix(ivy): proper resolution of Enums in Component decorator
1c46268
@mary-poppins
Copy link

You can preview 1c46268 at https://pr27971-1c46268.ngbuilds.io/.

alxhub
alxhub approved these changes Jan 9, 2019
View reviewed changes
Copy link
Member

@alxhub alxhub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with one comment.

packages/compiler-cli/src/ngtsc/annotations/src/util.ts Outdated Show resolved Hide resolved
alxhub
alxhub approved these changes Jan 9, 2019
View reviewed changes
Copy link
Member

@alxhub alxhub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with two comments*

packages/compiler-cli/src/ngtsc/annotations/src/component.ts
@@ -327,6 +329,24 @@ export class ComponentDecoratorHandler implements
return meta;
}

private _resolveEnumValue(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple points here.

  1. I would prefer if this function didn't include actually reading field from the Map. Passing the Map in just to read one field off of it seems unnecessary. Can you separate this logic?

  2. Return null and not undefined in ngtsc code to indicate that a value is logically not set.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review! Replies to your comments:

  1. The field is also used in that function in the error message in throw new FatalDiagnosticError to indicate which field has problems.

  2. The changeDetection field in metadata object that we construct later in that function requires either be defined as ChangeDetectionStartegy or be undefined. I wanted to avoid additional conversion from null -> undefined to satisfy the contract.

Please let me know if that works for you.

Thank you.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I still dislike the pattern, but understand the reasoning.

  2. That field (changeDetection on R3ComponentMetadata) was added incorrectly - it should be non-optional and non-nullable. We should always be passing a value for it.

If you feel like fixing this in this PR, that would be awesome. If not I can do it in a followup, but please do use null even if you have to convert it later for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Alex! I've created additional fixup commit (28bcd6a) to add changeDetection field only if it's not empty to satisfy the current contract. I've also created a ticket to update the code according to your proposal in followup PRs. Thank you.

@AndrewKushnir AndrewKushnir added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jan 9, 2019
@mary-poppins
Copy link

You can preview f1805f3 at https://pr27971-f1805f3.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 28bcd6a at https://pr27971-28bcd6a.ngbuilds.io/.

@AndrewKushnir
Copy link
Contributor Author

AndrewKushnir commented Jan 10, 2019 •
edited

Presubmit

@AndrewKushnir AndrewKushnir added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Jan 10, 2019
wKoza pushed a commit to wKoza/angular that referenced this pull request Jan 18, 2019
@AndrewKushnir @wKoza
fix(ivy): proper resolution of Enums in Component decorator (angular#…
309fc7b 
…27971)

Prior to this change Component decorator was resolving `encapsulation` value a bit incorrectly, which resulted in `encapsulation: NaN` in compiled code. Now we resolve the value as Enum memeber and throw if it's not the case. As a part of this update, the `changeDetection` field handling is also added, the resolution logic is the same as the one used for `encapsulation` field.

PR Close angular#27971
wKoza pushed a commit to wKoza/angular that referenced this pull request Jan 18, 2019
@AndrewKushnir @wKoza
fix(ivy): proper resolution of Enums in Component decorator (angular#…
dd3bf19 
…27971)

Prior to this change Component decorator was resolving `encapsulation` value a bit incorrectly, which resulted in `encapsulation: NaN` in compiled code. Now we resolve the value as Enum memeber and throw if it's not the case. As a part of this update, the `changeDetection` field handling is also added, the resolution logic is the same as the one used for `encapsulation` field.

PR Close angular#27971
ngfelixl pushed a commit to ngfelixl/angular that referenced this pull request Jan 28, 2019
@AndrewKushnir @ngfelixl
fix(ivy): proper resolution of Enums in Component decorator (angular#…
ec4153a 
…27971)

Prior to this change Component decorator was resolving `encapsulation` value a bit incorrectly, which resulted in `encapsulation: NaN` in compiled code. Now we resolve the value as Enum memeber and throw if it's not the case. As a part of this update, the `changeDetection` field handling is also added, the resolution logic is the same as the one used for `encapsulation` field.

PR Close angular#27971
@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 Sep 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@alxhub alxhub alxhub approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker cla: yes target: major This PR is targeted for the next major release type: bug/fix
Projects
None yet
Milestone
needsTriage
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@AndrewKushnir @mary-poppins @alxhub @googlebot