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

feat($compile): add one-way collection bindings #16553

Merged
merged 1 commit into from May 17, 2018
Merged

Conversation

jbedard
Copy link
Contributor

@jbedard jbedard commented May 4, 2018

Implements #14039

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.

One minor comment. LGTM otherwise.


expect($rootScope.collection[2]).toEqual(newItem);
}));
});
Copy link
Member

Choose a reason for hiding this comment

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

Missing a test to verify the $watchCollection part. (AFAICT, these tests would have passed even if * was ignored.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the first test verifies it, and this test is just wrong/not-applicable (you can tell I copied them both from the =* tests...).

Copy link
Member

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

Is it worth testing the $watchCollection is being used rather than a deep $watch?

@jbedard jbedard force-pushed the 14039 branch 2 times, most recently from 1380a9a to 299e137 Compare May 5, 2018 07:59
@jbedard
Copy link
Contributor Author

jbedard commented May 5, 2018

Updated the tests. Added the one @petebacondarwin suggested, removed the one @gkalpak pointed out was pointless, and added 2 which cause infdig with the non-collection version.

@jbedard
Copy link
Contributor Author

jbedard commented May 5, 2018

I wonder if =* should be deprecated since <* is exactly the same? Right now =* has a slightly more complicated action, but I'm pretty sure the logic/perf is the same, and #16550 actually makes it the exact same.

@gkalpak
Copy link
Member

gkalpak commented May 8, 2018

They shouldn't be the same. =* should update the parent if the child gets re-assigned, while <* shouldn't, right?

@jbedard
Copy link
Contributor Author

jbedard commented May 8, 2018

So you'd think... but it only does $watchCollection on the outer value. So when the inner value changes it won't do anything until the outer value changes, but at that time the outer value should be copied to the inner.

@petebacondarwin
Copy link
Member

No point in deprecating anything now @jbedard :-)

@gkalpak
Copy link
Member

gkalpak commented May 9, 2018

So when the inner value changes it won't do anything until the outer value changes

😱 😱 😱 Sounds like a bug in =*. So, essentially, we did support <* all along 😛

@jbedard
Copy link
Contributor Author

jbedard commented May 9, 2018

@petebacondarwin I was thinking it would be to encourage using <* instead, especially since =* is misleading

@gkalpak yeah... unless I'm missing something?

@jbedard
Copy link
Contributor Author

jbedard commented May 9, 2018

Updated the commit to also close #15874 in addition to #14039

@petebacondarwin petebacondarwin self-assigned this May 14, 2018
@Narretz
Copy link
Contributor

Narretz commented May 16, 2018

There's an eslint error in compile.js (doublequotes)

@jbedard
Copy link
Contributor Author

jbedard commented May 17, 2018

Fixed

@Narretz Narretz merged commit f9d1ca2 into angular:master May 17, 2018
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.

None yet

5 participants