fix(overlay): make config immutable for existing refs by jelbourn · Pull Request #7376 · angular/components

jelbourn · 2017-09-27T21:34:40Z

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.

crisbeto · 2017-09-27T21:39:49Z

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];
};

jelbourn · 2017-09-27T22:15:33Z

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.

crisbeto · 2017-09-27T22:20:00Z

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

crisbeto · 2017-09-28T05:45:00Z

src/cdk/overlay/overlay-ref.ts

 import {Subject} from 'rxjs/Subject';
+import {OverlayConfig} from './overlay-config';

+export type ImmutableObject<T> = {


Add a description here?

crisbeto · 2017-09-28T05:45:38Z

src/cdk/overlay/overlay-ref.ts

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


It's not cloning it anymore.

crisbeto · 2017-09-28T05:46:16Z

src/cdk/overlay/overlay-ref.ts

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


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

Because the config is immutable

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

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.

crisbeto

LGTM

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.

angular-automatic-lock-bot · 2019-09-08T15:02:40Z

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

jelbourn added the pr: needs review label Sep 27, 2017

jelbourn requested a review from crisbeto September 27, 2017 21:34

jelbourn requested review from josephperrott, kara and mmalerba as code owners September 27, 2017 21:34

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

jelbourn force-pushed the overlayref branch from 3b49387 to bc03cc6 Compare September 27, 2017 21:35

angular deleted a comment from googlebot Sep 27, 2017

jelbourn force-pushed the overlayref branch from bc03cc6 to 3bbbb17 Compare September 27, 2017 22:14

angular deleted a comment from googlebot Sep 27, 2017

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

angular deleted a comment from googlebot Sep 27, 2017

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 reviewed Sep 28, 2017

View reviewed changes

angular deleted a comment from googlebot Sep 28, 2017

jelbourn force-pushed the overlayref branch from 3bbbb17 to 6b9a3c3 Compare September 28, 2017 16:10

crisbeto approved these changes Sep 28, 2017

View reviewed changes

angular deleted a comment from googlebot Sep 28, 2017

andrewseguin added pr: needs rebase and removed action: merge The PR is ready for merge by the caretaker labels Sep 29, 2017

jelbourn force-pushed the overlayref branch from 6b9a3c3 to eaa79ef Compare November 14, 2017 17:57

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 removed the cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla label Nov 14, 2017

jelbourn force-pushed the overlayref branch from eaa79ef to 4c88015 Compare November 14, 2017 17:59

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 deleted a comment from googlebot Nov 14, 2017

jelbourn added this to the 5.0 milestone Nov 19, 2017

jelbourn force-pushed the overlayref branch from 4c88015 to 07d6940 Compare November 22, 2017 15:39

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 deleted a comment from googlebot Nov 22, 2017

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 force-pushed the overlayref branch from 07d6940 to cbf47a7 Compare November 22, 2017 15:42

jelbourn mentioned this pull request Nov 27, 2017

fix(menu): update position on every open #8653

Closed

tinayuangao merged commit 2dbc766 into angular:master Nov 28, 2017

tinayuangao mentioned this pull request Dec 5, 2017

Revert "fix(overlay): make config immutable for existing refs" #8806

Closed

jelbourn deleted the overlayref branch April 2, 2018 22:31

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 8, 2019

Conversation

jelbourn commented Sep 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crisbeto commented Sep 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jelbourn commented Sep 27, 2017

Uh oh!

crisbeto commented Sep 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crisbeto left a comment

Choose a reason for hiding this comment

Uh oh!

angular-automatic-lock-bot bot commented Sep 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jelbourn commented Sep 27, 2017 •

edited

Loading

crisbeto commented Sep 27, 2017 •

edited

Loading