fix+perf($parse): allow watching array/object literal values, disable deep watch for one-way bindings #15301

jbedard · 2016-10-20T09:18:37Z

I think this is a better alternative to #15274, but has a breaking change (which didn't break any tests!).

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

What is the current behavior? (You can also link to an open issue here)
Watching literals doesn't always work, literals passed to components are deep watched to work around this.

What is the new behavior (if this is a feature change)?
Watching literals now does a watch of the inputs.

Does this PR introduce a breaking change?
YES - see below or second commit

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

BREAKING CHANGE (65ff861):
Previously when a literal value was passed into a directive/component via one-way binding it would be watched with a deep watcher.

For example, for <my-component input="[a]">, a new instance of the array would be passed into the directive/component (and trigger $onChanges) not only if a changed but also if any sub property of a changed such as a.b or a.b.c.d.e etc.

This also means a new but equal value for a would NOT trigger such a change.

Now literal values use a non-deep watch similar to other directive/component one-way bindings. Changes are only trigger when the value itself changes.

Avoiding deep watchers for array/object literals will improve watcher performance of all literals passed as one-way bindings, especially those containing references to large/complex objects.

- the content of array/object literals is now watched for changes - if the content changes then a new array/object will be constructed

…r literals BREAKING CHANGE: Previously when a literal value was passed into a directive/component via one-way binding it would be watched with a deep watcher. For example, for `<my-component input="[a]">`, a new instance of the array would be passed into the directive/component (and trigger $onChanges) not only if `a` changed but also if any sub property of `a` changed such as `a.b` or `a.b.c.d.e` etc. This also means a new but equal value for `a` would NOT trigger such a change. Now literal values use a non-deep watch similar to other directive/component one-way bindings. Changes are only trigger when the value itself changes. Avoiding deep watchers for array/object literals will improve watcher performance of all literals passed as one-way bindings, especially those containing references to large/complex objects.

gkalpak · 2016-10-20T18:06:38Z

Seems like a good thing (it has to wait for 1.7.0 though).
May be worth documenting - may be not.

thorn0 · 2016-10-22T10:50:46Z

src/ng/compile.js

@@ -3522,7 +3522,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
              }
              recordChanges(scopeName, newValue, oldValue);
              destination[scopeName] = newValue;
-            }, deepWatch);


Deep watching never happens, but we still have a variable named deepWatch. Isn't it a bit misleading?

OK so let's change deepWatch to isLiteral.

petebacondarwin

I think this looks good. I would like to see a test in compileSpec.js that demonstrates that we are no longer deep watching - just for the record.

petebacondarwin · 2017-01-25T13:50:50Z

src/ng/compile.js

@@ -3522,7 +3522,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
              }
              recordChanges(scopeName, newValue, oldValue);
              destination[scopeName] = newValue;
-            }, deepWatch);


OK so let's change deepWatch to isLiteral.

petebacondarwin

So I guess I shouldn't approve this since I have asked for two changes:

rename deepWatch local variable to isLiteral
add test to compileSpec.js

jbedard · 2017-01-27T09:06:19Z

Renamed the var and added some tests. I think those should both be squashed with the second commit.

Note that c175619 (first commit) can actually go into 1.6 since it only adds support for literal-watching to $parse (without any use of equals like #15274 did). Then 65ff861 has the breaking change - dropping equals in one-way bindings.

gkalpak

I wonder how this change affects watching object literals with computed property names.

gkalpak · 2017-01-27T09:49:36Z

test/ng/compileSpec.js

@@ -4259,6 +4259,78 @@ describe('$compile', function() {
          });


+          it('should trigger `$onChanges` for literal expressions when expression input value changes (simple value)', function() {


Can you also add tests verifying that $onChanges is not called when a property of the input value changes?

petebacondarwin

Great. Thanks @jbedard
I will squash and merge. The first commit should probably be a feat, right? And I agree that it can go into 1.6

The inputs of array/object literals are now watched for changes. If the an input changes then a new instance of the literal will be provided when the parsed expression is executed. Closes #15301

Avoiding deep watchers for array/object literals will improve watcher performance of all literals passed as one-way bindings, especially those containing references to large/complex objects. BREAKING CHANGE: Previously when a literal value was passed into a directive/component via one-way binding it would be watched with a deep watcher. For example, for `<my-component input="[a]">`, a new instance of the array would be passed into the directive/component (and trigger $onChanges) not only if `a` changed but also if any sub property of `a` changed such as `a.b` or `a.b.c.d.e` etc. This also means a new but equal value for `a` would NOT trigger such a change. Now literal values use an input-based watch similar to other directive/component one-way bindings. In this context inputs are the non-constant parts of the literal. In the example above the input would be `a`. Changes are only triggered when the inputs to the literal change. Closes #15301

The inputs of array/object literals are now watched for changes. If the an input changes then a new instance of the literal will be provided when the parsed expression is executed. Closes angular#15301

Avoiding deep watchers for array/object literals will improve watcher performance of all literals passed as one-way bindings, especially those containing references to large/complex objects. BREAKING CHANGE: Previously when a literal value was passed into a directive/component via one-way binding it would be watched with a deep watcher. For example, for `<my-component input="[a]">`, a new instance of the array would be passed into the directive/component (and trigger $onChanges) not only if `a` changed but also if any sub property of `a` changed such as `a.b` or `a.b.c.d.e` etc. This also means a new but equal value for `a` would NOT trigger such a change. Now literal values use an input-based watch similar to other directive/component one-way bindings. In this context inputs are the non-constant parts of the literal. In the example above the input would be `a`. Changes are only triggered when the inputs to the literal change. Closes angular#15301

jbedard added 2 commits October 20, 2016 01:59

fix($parse): allow watching array/object literal values

c175619

- the content of array/object literals is now watched for changes - if the content changes then a new array/object will be constructed

googlebot added the cla: yes label Oct 20, 2016

gkalpak added type: feature frequency: moderate needs: breaking change component: $parse type: perf severity: inconvenient labels Oct 20, 2016

gkalpak added this to the Backlog milestone Oct 20, 2016

thorn0 reviewed Oct 22, 2016

View reviewed changes

petebacondarwin modified the milestones: 1.7.0, Backlog Jan 23, 2017

petebacondarwin self-requested a review January 23, 2017 20:26

petebacondarwin approved these changes Jan 25, 2017

View reviewed changes

petebacondarwin suggested changes Jan 25, 2017

View reviewed changes

jbedard mentioned this pull request Jan 27, 2017

fix($parse): allow watching object/array literals #15274

Closed

3 tasks

jbedard added 2 commits January 27, 2017 00:51

fixup! add tests verifying one-way watch does not use deep-equals

3d81cee

fixup! rename deepWatch variable

0706760

jbedard force-pushed the watch-literals-nodeepequals branch from 1fecb6f to 0706760 Compare January 27, 2017 08:53

gkalpak reviewed Jan 27, 2017

View reviewed changes

petebacondarwin approved these changes Jan 27, 2017

View reviewed changes

petebacondarwin closed this in 7084dec Jan 27, 2017

gkalpak mentioned this pull request Feb 16, 2017

Angular binding causing Maximum call stack size exceeded #15712

Closed

gkalpak mentioned this pull request Mar 20, 2017

Unintended breaking change when passing ngModel as a binding #15833

Closed

dependencies bot mentioned this pull request May 11, 2018

Update 3 dependencies from npm JetBrains/ring-ui#257

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix+perf($parse): allow watching array/object literal values, disable deep watch for one-way bindings #15301

fix+perf($parse): allow watching array/object literal values, disable deep watch for one-way bindings #15301

jbedard commented Oct 20, 2016

gkalpak commented Oct 20, 2016

thorn0 Oct 22, 2016

petebacondarwin Jan 25, 2017

petebacondarwin left a comment

petebacondarwin Jan 25, 2017

petebacondarwin left a comment

jbedard commented Jan 27, 2017

gkalpak left a comment

gkalpak Jan 27, 2017

petebacondarwin left a comment

		@@ -4259,6 +4259,78 @@ describe('$compile', function() {
		});


		it('should trigger `$onChanges` for literal expressions when expression input value changes (simple value)', function() {

fix+perf($parse): allow watching array/object literal values, disable deep watch for one-way bindings #15301

fix+perf($parse): allow watching array/object literal values, disable deep watch for one-way bindings #15301

Conversation

jbedard commented Oct 20, 2016

gkalpak commented Oct 20, 2016

thorn0 Oct 22, 2016

Choose a reason for hiding this comment

petebacondarwin Jan 25, 2017

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

petebacondarwin Jan 25, 2017

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

jbedard commented Jan 27, 2017

gkalpak left a comment

Choose a reason for hiding this comment

gkalpak Jan 27, 2017

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment