Skip to content

Conversation

@mprobst
Copy link
Contributor

@mprobst mprobst commented Jun 18, 2018

OverlayRef structurally matches OverlayReference, but does not
explicitly implement it. That works in TypeScript, and should work in
Closure Compiler through the use of @record, but for unclear reasons
doesn't.

Explicitly implementing the interface makes this code clearer, and works
around the problem in Closure Compiler while that is being fixed.

`OverlayRef` structurally matches `OverlayReference`, but does not
explicitly implement it. That works in TypeScript, and should work in
Closure Compiler through the use of `@record`, but for unclear reasons
doesn't.

Explicitly implementing the interface makes this code clearer, and works
around the problem in Closure Compiler while that is being fixed.
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jun 18, 2018
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

LGTM. IIRC I didn't do this in #9907 initially, because we still had a reference to PositionStrategy somewhere in OverlayRef which would've introduced a circular import, but it doesn't seem to be the case anymore.

@crisbeto crisbeto added pr: lgtm action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels Jun 18, 2018
@mprobst
Copy link
Contributor Author

mprobst commented Jun 18, 2018

Thanks for the quick review!

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

@jelbourn jelbourn added the P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful label Jun 18, 2018
@tinayuangao tinayuangao merged commit 8050f63 into angular:master Jun 26, 2018
tinayuangao pushed a commit that referenced this pull request Jun 26, 2018
`OverlayRef` structurally matches `OverlayReference`, but does not
explicitly implement it. That works in TypeScript, and should work in
Closure Compiler through the use of `@record`, but for unclear reasons
doesn't.

Explicitly implementing the interface makes this code clearer, and works
around the problem in Closure Compiler while that is being fixed.
victoriaaa234 pushed a commit that referenced this pull request Jul 25, 2018
`OverlayRef` structurally matches `OverlayReference`, but does not
explicitly implement it. That works in TypeScript, and should work in
Closure Compiler through the use of `@record`, but for unclear reasons
doesn't.

Explicitly implementing the interface makes this code clearer, and works
around the problem in Closure Compiler while that is being fixed.
@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 9, 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 PR author has agreed to Google's Contributor License Agreement P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants