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(overlay): make config immutable for existing refs #7376

Merged
merged 1 commit into from
Nov 28, 2017
Merged

fix(overlay): make config immutable for existing refs #7376

merged 1 commit into from
Nov 28, 2017

Conversation

jelbourn
Copy link
Member

@jelbourn jelbourn commented Sep 27, 2017 •
edited
Loading

This changes makes the OverlayConfig instance in OverlatRef immutable.
Calling getConfig will now return a clone of the config to make clear
that it cannot be modified directly to change the state of the overlay.
This also updates the updateSize method to accept a partial config to
apply to the existing config (and make a new config instance)

BREAKING CHANGE: OverlayRef.getConfig returns a clone of the config
object.

BREAKING CHANGE: OverlayRef.updateSize now accepts a OverlaySizeConfig
rather than being based on the existing config object.

@googlebot googlebot added the cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla label Sep 27, 2017
@crisbeto
Copy link
Member

crisbeto commented Sep 27, 2017 •
edited
Loading

Rather than cloning and ensuring that everything is immutable at runtime, I think that we can get away with making it immutable in TS. We can add a Readonly type and return Readonly<OverlayRef>. The type looks something like this:

type Readonly<T> = {
  readonly [P in keyof T]: T[P];
};

@angular angular deleted a comment from googlebot Sep 27, 2017
@angular angular deleted a comment from googlebot Sep 27, 2017
@jelbourn
Copy link
Member Author

Added an ImmutableObject type (I avoided Readonly since that's already a separate thing in typescript). This actually caught a place in overlay-directives where we were directly writing the direction.

@jelbourn jelbourn added cla: yes PR author has agreed to Google's Contributor License Agreement and removed cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla labels Sep 27, 2017
@crisbeto
Copy link
Member

Yeah I forgot to mention that there may be some places that depend on being able to update properties dynamically (e.g. I think the overlay directives do it in case the direction changed between attachments).

@angular angular deleted a comment from googlebot Sep 27, 2017
@googlebot googlebot added cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla and removed cla: yes PR author has agreed to Google's Contributor License Agreement labels Sep 28, 2017
crisbeto
crisbeto reviewed Sep 28, 2017
View reviewed changes
src/cdk/overlay/overlay-ref.ts Outdated

export type ImmutableObject<T> = {
Copy link
Member

Choose a reason for hiding this comment

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

Add a description here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

src/cdk/overlay/overlay-ref.ts Outdated
return this._state;
/** Gets the the current overlay configuration, which is immutable. */
getConfig(): ImmutableObject<OverlayConfig> {
// Clone the config so that it cannot be modified outside of this class.
Copy link
Member

Choose a reason for hiding this comment

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

It's not cloning it anymore.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

src/cdk/overlay/overlay-ref.ts

/** Sets the LTR/RTL direction for the overlay. */
setDirection(dir: Direction) {
this._config = {...this._config, direction: dir};
Copy link
Member

Choose a reason for hiding this comment

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

Why clone the config here instead of just setting the direction?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because the config is immutable

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't it be enough to clone it on init and then work off the clone?

Copy link
Member Author

Choose a reason for hiding this comment

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

I like having the config as immutable all the time because it reinforces the fact that something extra needs to be done to update it.

@angular angular deleted a comment from googlebot Sep 28, 2017
crisbeto
crisbeto approved these changes Sep 28, 2017
View reviewed changes
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

@crisbeto crisbeto added pr: lgtm action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement and removed pr: needs review cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla labels Sep 28, 2017
@angular angular deleted a comment from googlebot Sep 28, 2017
@andrewseguin andrewseguin added pr: needs rebase and removed action: merge The PR is ready for merge by the caretaker labels Sep 29, 2017
@googlebot googlebot added cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla and removed cla: yes PR author has agreed to Google's Contributor License Agreement labels Nov 14, 2017
@jelbourn jelbourn removed the cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla label Nov 14, 2017
@jelbourn jelbourn added action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement and removed pr: needs rebase labels Nov 14, 2017
@angular angular deleted a comment from googlebot Nov 14, 2017
@jelbourn jelbourn added this to the 5.0 milestone Nov 19, 2017
@googlebot googlebot added cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla and removed cla: yes PR author has agreed to Google's Contributor License Agreement labels Nov 22, 2017
@angular angular deleted a comment from googlebot Nov 22, 2017
@jelbourn jelbourn added cla: yes PR author has agreed to Google's Contributor License Agreement and removed cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla labels Nov 22, 2017
@jelbourn
fix(overlay): make config immutable for existing refs
cbf47a7 
This changes makes the OverlayConfig instance in OverlatRef immutable.
Calling `getConfig` will now return a clone of the config to make clear
that it cannot be modified directly to change the state of the overlay.
This also updates the `updateSize` method to accept a partial config to
apply to the existing config (and make a new config instance).

This *also* tightens up the typings on Portal.attach

BREAKING CHANGE: OverlayRef.getConfig returns an immutable version of
the config object.

BREAKING CHANGE: OverlayRef.updateSize now accepts a OverlaySizeConfig
rather than being based on the existing config object.
@jelbourn jelbourn mentioned this pull request Nov 27, 2017
Closed
@tinayuangao tinayuangao merged commit 2dbc766 into angular:master Nov 28, 2017
tinayuangao pushed a commit to tinayuangao/material2 that referenced this pull request Nov 29, 2017
@jelbourn @tinayuangao
fix(overlay): make config immutable for existing refs (angular#7376)
3e53816 
This changes makes the OverlayConfig instance in OverlatRef immutable.
Calling `getConfig` will now return a clone of the config to make clear
that it cannot be modified directly to change the state of the overlay.
This also updates the `updateSize` method to accept a partial config to
apply to the existing config (and make a new config instance).

This *also* tightens up the typings on Portal.attach

BREAKING CHANGE: OverlayRef.getConfig returns an immutable version of
the config object.

BREAKING CHANGE: OverlayRef.updateSize now accepts a OverlaySizeConfig
rather than being based on the existing config object.
@tinayuangao tinayuangao mentioned this pull request Dec 5, 2017
Closed
@jelbourn jelbourn deleted the overlayref branch April 2, 2018 22:31
@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 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@crisbeto crisbeto crisbeto approved these changes

@josephperrott josephperrott Awaiting requested review from josephperrott

@kara kara Awaiting requested review from kara

@mmalerba mmalerba Awaiting requested review from mmalerba

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Milestone
5.0
Development

Successfully merging this pull request may close these issues.
5 participants
@jelbourn @crisbeto @googlebot @tinayuangao @andrewseguin