Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix($parse): Make sure ES6 object computed property to be watched #15637

Closed

Conversation

kindy
Copy link
Contributor

@kindy kindy commented Jan 24, 2017

Adding the missing watches for ES6 object property which added in #14407

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix

What is the current behavior? (You can also link to an open issue here)
$scope.$watch can not work as expected.

What is the new behavior (if this is a feature change)?
$scope.$watch works (NOT a feature change)

Does this PR introduce a breaking change?
NO

Please check if the PR fulfills these requirements

Other information:

Adding the missing watches for ES6 object property which added in angular#14407
it('should watch ES6 object computed property changes', function() {
var count = 0;
var values = [];
var firstValue = {'undefined': true};
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem necessary. I think it's better to inline the object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gkalpak fixed.

var values = [];
var firstValue = {'undefined': true};

scope.$watch('{[a]: true}', function(val, oldVal) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Remove unused arg.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gkalpak fixed.

@kindy
Copy link
Contributor Author

kindy commented Jan 25, 2017

@gkalpak because I'm using v1.5.x now, so I just submit this PR to v1.5.x .
do you think I need create another PR for v1.6.x ?

@Narretz
Copy link
Contributor

Narretz commented Jan 25, 2017

@kindy we are branching and merging bug fixes from / to master and backport them manually to 1.5.x, so yes a PR that branches from master is preferred. Just open a new one with the updated code.

Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

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

LGTM, but ideally @lgalfaso should take a look too 😃

@kindy
Copy link
Contributor Author

kindy commented Feb 4, 2017

@lgalfaso can you help review this PR?
Thanks ~

@lgalfaso
Copy link
Contributor

lgalfaso commented Feb 4, 2017 via email

@gkalpak
Copy link
Member

gkalpak commented Feb 4, 2017

@kindy, can you please rebase this on master?
(Actually, the PR should be againt master. We can backport to the necessary branches by cherry-picking from there.)

@kindy
Copy link
Contributor Author

kindy commented Feb 4, 2017

@gkalpak @Narretz
the same PR for master is here: #15678 .

@gkalpak
Copy link
Member

gkalpak commented Feb 4, 2017

Thx! Closing in favor of #15678.

@gkalpak gkalpak closed this Feb 4, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants