refactor(core): refactor WrappedValue #20997

vicb · 2017-12-14T00:23:03Z

Improve WrappedValue by adding unwrap symetrical to wrap.
remove dead code - ValueUnwrapper

The property wrapped is an implementation details and should never be accessed directly - use unwrap(wrappedValue). Will change to protected in Angular 7.

mary-poppins · 2017-12-14T00:30:28Z

You can preview af5ad97 at https://pr20997-af5ad97.ngbuilds.io/.

mary-poppins · 2017-12-14T00:36:48Z

You can preview 8728965 at https://pr20997-8728965.ngbuilds.io/.

chuckjaz · 2017-12-14T00:46:10Z

packages/core/src/change_detection/change_detection_util.ts


-  reset() { this.hasWrappedValue = false; }
+  static isWrapped(value: any): boolean { return value instanceof WrappedValue; }


Consider changing the return type to value is WrappedValue and then changing unwrap to us it.

mary-poppins · 2017-12-14T01:09:35Z

You can preview 33dc163 at https://pr20997-33dc163.ngbuilds.io/.

mary-poppins · 2017-12-14T19:38:52Z

You can preview b508bb4 at https://pr20997-b508bb4.ngbuilds.io/.

vicb · 2017-12-14T21:28:25Z

status: await approveApiChange(); merge();

IgorMinar

otherwise lgtm

IgorMinar · 2017-12-14T22:35:57Z

packages/core/src/change_detection/change_detection_util.ts

-    }
-    return value;
-  }
+  static unwrap(value: any): any { return WrappedValue.isWrapped(value) ? value.wrapped : value; }


please add docs

IgorMinar · 2017-12-14T22:40:58Z

also please add better commit message description to explain the motivation behind this change.

mary-poppins · 2017-12-15T00:34:46Z

You can preview c662dca at https://pr20997-c662dca.ngbuilds.io/.

mary-poppins · 2017-12-15T01:08:00Z

You can preview fb3cae2 at https://pr20997-fb3cae2.ngbuilds.io/.

IgorMinar

Small tweaks requested. Otherwise lgtm.

IgorMinar · 2017-12-15T01:32:34Z

packages/core/src/change_detection/change_detection_util.ts


-  reset() { this.hasWrappedValue = false; }
+  /** Return whether `value` is wrapped */


Returns true if value is wrapped.

IgorMinar · 2017-12-15T01:33:03Z

packages/core/src/change_detection/change_detection_util.ts

-    }
-    return value;
-  }
+  /** Return the underlying value of a wrapped value. Returns the given `value` when it is not


Return - > Returns

And can you please move the second sentence on a new line?

IgorMinar · 2017-12-15T01:34:51Z

packages/core/src/change_detection/change_detection_util.ts

- */
-export class ValueUnwrapper {
-  public hasWrappedValue = false;
+  /** Creates a wrapped value */


mary-poppins · 2017-12-15T02:58:01Z

You can preview 715405f at https://pr20997-715405f.ngbuilds.io/.

mary-poppins · 2017-12-15T02:58:18Z

You can preview bbf24ee at https://pr20997-bbf24ee.ngbuilds.io/.

IgorMinar · 2017-12-15T04:28:58Z

packages/core/src/change_detection/change_detection_util.ts

@@ -44,26 +44,22 @@ export function devModeEqual(a: any, b: any): boolean {
 * @stable
 */
 export class WrappedValue {
-  constructor(public wrapped: any) {}
+  /** @deprecated from 6.0, use `unwrap()` instead - will switch to protected */


This should be 5.2

IgorMinar · 2017-12-15T19:20:19Z

@vicb you said that this commit doesn't need a better description, yet you created an awesome description for the PR. I think the same info that's in the PR description would be useful to anyone looking at the commit via git log. Can you please copy it there?

In general we should care more about the commit messages than PR descriptions because we rarely go back to look at the old PRs but we more commonly look at individual commits when doing git log or doing release review. thanks!

- Improve `WrappedValue` by adding `unwrap` symetrical to `wrap`. - remove dead code - `ValueUnwrapper` The property `wrapped` is an implementation details and should never be accessed directly - use `unwrap(wrappedValue)`. Will change to protected in Angular 7.

mary-poppins · 2018-01-11T17:36:25Z

You can preview 4a9c06c at https://pr20997-4a9c06c.ngbuilds.io/.

mary-poppins · 2018-01-11T17:37:59Z

You can preview d1adacc at https://pr20997-d1adacc.ngbuilds.io/.

- Improve `WrappedValue` by adding `unwrap` symetrical to `wrap`. - remove dead code - `ValueUnwrapper` The property `wrapped` is an implementation details and should never be accessed directly - use `unwrap(wrappedValue)`. Will change to protected in Angular 7. PR Close #20997

- Improve `WrappedValue` by adding `unwrap` symetrical to `wrap`. - remove dead code - `ValueUnwrapper` The property `wrapped` is an implementation details and should never be accessed directly - use `unwrap(wrappedValue)`. Will change to protected in Angular 7. PR Close angular#20997

angular-automatic-lock-bot · 2019-09-13T06:21:08Z

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

googlebot added the cla: yes label Dec 14, 2017

vicb force-pushed the 1213-wrap branch from af5ad97 to 8728965 Compare December 14, 2017 00:29

vicb added area: core Issues related to the framework runtime action: review The PR is still awaiting reviews from at least one requested reviewer refactoring Issue that involves refactoring or code-cleanup target: patch This PR is targeted for the next patch release labels Dec 14, 2017

chuckjaz suggested changes Dec 14, 2017

View reviewed changes

vicb force-pushed the 1213-wrap branch from 8728965 to 33dc163 Compare December 14, 2017 01:02

vicb force-pushed the 1213-wrap branch from 33dc163 to b508bb4 Compare December 14, 2017 19:29

chuckjaz approved these changes Dec 14, 2017

View reviewed changes

vicb 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 Dec 14, 2017

IgorMinar approved these changes Dec 14, 2017

View reviewed changes

IgorMinar 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: merge The PR is ready for merge by the caretaker labels Dec 14, 2017

vicb force-pushed the 1213-wrap branch from b508bb4 to c662dca Compare December 15, 2017 00:28

vicb force-pushed the 1213-wrap branch from c662dca to fb3cae2 Compare December 15, 2017 00:59

IgorMinar approved these changes Dec 15, 2017

View reviewed changes

vicb force-pushed the 1213-wrap branch 4 times, most recently from 715405f to bbf24ee Compare December 15, 2017 02:51

IgorMinar reviewed Dec 15, 2017

View reviewed changes

vicb force-pushed the 1213-wrap branch from bbf24ee to 4a9c06c Compare January 11, 2018 17:28

vicb force-pushed the 1213-wrap branch from 4a9c06c to d1adacc Compare January 11, 2018 17:30

vicb 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 Jan 11, 2018

IgorMinar approved these changes Jan 11, 2018

View reviewed changes

IgorMinar added the action: merge The PR is ready for merge by the caretaker label Jan 11, 2018

alexeagle closed this in 3bcc0e6 Jan 16, 2018

vicb deleted the 1213-wrap branch January 29, 2018 19:25

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(core): refactor WrappedValue #20997

refactor(core): refactor WrappedValue #20997

vicb commented Dec 14, 2017 •

edited

mary-poppins commented Dec 14, 2017

mary-poppins commented Dec 14, 2017

chuckjaz Dec 14, 2017

mary-poppins commented Dec 14, 2017

mary-poppins commented Dec 14, 2017

vicb commented Dec 14, 2017

IgorMinar left a comment

IgorMinar Dec 14, 2017

IgorMinar commented Dec 14, 2017

mary-poppins commented Dec 15, 2017

mary-poppins commented Dec 15, 2017

IgorMinar left a comment

IgorMinar Dec 15, 2017

IgorMinar Dec 15, 2017

IgorMinar Dec 15, 2017

IgorMinar Dec 15, 2017

mary-poppins commented Dec 15, 2017

mary-poppins commented Dec 15, 2017

IgorMinar Dec 15, 2017

IgorMinar commented Dec 15, 2017

mary-poppins commented Jan 11, 2018

mary-poppins commented Jan 11, 2018

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


		reset() { this.hasWrappedValue = false; }
		static isWrapped(value: any): boolean { return value instanceof WrappedValue; }


		reset() { this.hasWrappedValue = false; }
		/** Return whether `value` is wrapped */

refactor(core): refactor WrappedValue #20997

refactor(core): refactor WrappedValue #20997

Conversation

vicb commented Dec 14, 2017 • edited

mary-poppins commented Dec 14, 2017

mary-poppins commented Dec 14, 2017

chuckjaz Dec 14, 2017

Choose a reason for hiding this comment

mary-poppins commented Dec 14, 2017

mary-poppins commented Dec 14, 2017

vicb commented Dec 14, 2017

IgorMinar left a comment

Choose a reason for hiding this comment

IgorMinar Dec 14, 2017

Choose a reason for hiding this comment

IgorMinar commented Dec 14, 2017

mary-poppins commented Dec 15, 2017

mary-poppins commented Dec 15, 2017

IgorMinar left a comment

Choose a reason for hiding this comment

IgorMinar Dec 15, 2017

Choose a reason for hiding this comment

IgorMinar Dec 15, 2017

Choose a reason for hiding this comment

IgorMinar Dec 15, 2017

Choose a reason for hiding this comment

IgorMinar Dec 15, 2017

Choose a reason for hiding this comment

mary-poppins commented Dec 15, 2017

mary-poppins commented Dec 15, 2017

IgorMinar Dec 15, 2017

Choose a reason for hiding this comment

IgorMinar commented Dec 15, 2017

mary-poppins commented Jan 11, 2018

mary-poppins commented Jan 11, 2018

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

vicb commented Dec 14, 2017 •

edited