Updating track by $index usage #16334

michael-letcher · 2017-11-20T14:37:11Z

Updating docs to be more clear about the pitfalls of track by $index

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

What is the current behavior? (You can also link to an open issue here)
NA

What is the new behavior (if this is a feature change)?
NA

Does this PR introduce a breaking change?
No

Please check if the PR fulfills these requirements

The commit message follows our guidelines
Fix/Feature: Docs have been added/updated
Fix/Feature: Tests have been added; existing tests pass

Other information:
REF #16332

googlebot · 2017-11-20T14:37:14Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

googlebot · 2017-11-20T14:37:15Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

michael-letcher · 2017-11-20T14:40:43Z

I signed it!

googlebot · 2017-11-20T14:40:46Z

CLAs look good, thanks!

googlebot · 2017-11-20T14:40:46Z

CLAs look good, thanks!

frederikprijck · 2017-11-20T14:48:24Z

src/ng/directive/ngRepeat.js

- * will not be updated even when the corresponding item changes, essentially causing the view to get
- * out-of-sync with the underlying data.
+ * {@link guide/expression#one-time-binding one-time bindings} or {@link guide/directive#creating-directives directives}. 
+ * In such cases, where the array changes and `track by $index` is in use the `nth` DOM element 


This doesn't look like it specifies what you mean, coz this works fine: https://plnkr.co/edit/dxzgjmE5MEA0bwQdckVS?p=preview aswell as https://plnkr.co/edit/2i0WKKzh1uDlj4kOrBFN?p=preview

Even tho I'm changing the array.

https://plnkr.co/edit/ytQ9p6YJOfrDubCGIc2N?p=preview

Click the remove and you will see the the UI is not updated and that the fail becomes green.

I know, it just that making a simple change to ur plunkr solves the issue: https://plnkr.co/edit/d39OLmANDGv8EuxXxA6k?p=preview (I replaced splice with slice)

Hence it's not very clear that this is the problem with arrays. (basicly I guess it's immutable vs changing the existing array)

Interesting, what does $filter use? As this is what we are using that causes this issue.

$filter uses Array.prototype.filter, which also doesn't manipulate the array but instead returns a new array. See: https://github.com/angular/angular.js/blob/master/src/ng/filter/filter.js#L172

michael-letcher · 2017-11-20T16:52:05Z

Messed up the commit messages, unsure what the process is to fix this. (not going to use the git cli unless specified as the documentation does not say what to do if missed)

<type>Docs: Updating track by $index usage <subject>

<body>
Updating docs to be more clear about the pitfalls of track by $index
<footer>

googlebot · 2017-11-20T19:52:13Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

googlebot · 2017-11-20T19:52:13Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

petebacondarwin · 2017-11-20T19:53:15Z

@michael-letcher I pulled down your branch, squashed the commits and force-pushed back to your branch. This has the effect of updating this PR.

Clarify the pitfalls of using `track by $index` Closes #16332

frederikprijck · 2017-11-20T20:14:52Z

Avoid using track by $index when the repeated template contains ...

This reads as if it's fine to use in all other situations.

Despite the fact I agree track by $index can be documented better as it currently is, I'm not sure whether it's something that's supposed to be promoted. Wouldn't it be a better idea to mention it's to be avoided (or adviced not to be used), unless you actually know what you're using it for (and mention some of it's downsides)?

Theoretically, it'll bring in a performance boost. Practically it's not something you want to be using, right ?

Wouldn't it be more beneficial to focus on tracking by something useful instead? To be fair, what's to be said of track by $index that would make u use it in most applications?

michael-letcher · 2017-11-21T09:46:15Z

@frederikprijck I agree, should I change the documentation to reflect this?

@gkalpak Also mentioned this:

in practice, tracking by $index is rarely useful

petebacondarwin · 2017-11-21T10:25:52Z

@michael-letcher - please do update to reflect this. You will need to pull down the latest commits from your branch before you try to make changes, since I pushed to your branch when I squashed.

git checkut patch-1
git pull

This should update everything locally (and there should be a message about how the branch had been force pushed).

Then when you have made the change be sure to amend the current commit, in the terminal you would use git commit --amend.

Narretz · 2017-11-21T11:46:42Z

track by $index is perfectly usable when you have only simple interpolation / expressions in the template, but not one-time bindings, or directives - all content that is static. I agree that we should only mention track by $index at the end of the section, with all the current and extra caveats. At the moment, we lead with it, which is bad.

frederikprijck · 2017-11-21T12:02:24Z

@Narretz I read this often, yet I added working plunkers with track by $index and updates to the underlying data used which seem to work properly. I'm abit confused why tho...

https://plnkr.co/edit/dxzgjmE5MEA0bwQdckVS?p=preview
https://plnkr.co/edit/2i0WKKzh1uDlj4kOrBFN?p=preview
https://plnkr.co/edit/d39OLmANDGv8EuxXxA6k?p=preview

track by $index is perfectly usable when you have only simple interpolation / expressions in the template

It's usable, but doesn't make alot of sense to track by an index in an array in most cases.

I agree that we should only mention track by $index at the end of the section

Ye, and focus on alternatives, like tracking by a property (even tho that doesn't have to be part of this PR I guess)

franciscovelez · 2017-11-21T14:52:47Z

@michael-letcher I just changed one line in the docs in the past. If you need my consent for this change, I'll give it to you. Should I need to do anything more?

Narretz · 2017-11-21T15:00:19Z

@franciscovelez I think @michael-letcher mistakenly @-ed you. I guess he wanted to @frederikprijck

michael-letcher · 2017-11-21T15:47:55Z

@Narretz I did, whoops!

I'll review the documentation again, and perhaps reorder the notion of track by to clearly state what Angular will be doing with it e.g. keep func. that is what gives the performance boost and not mention that using $index gives the benefit.
Also giving some nice examples of this would be good, one tracking by ID with directives and one using index, perhaps?

franciscovelez · 2017-11-22T08:14:57Z

@franciscovelez I think @michael-letcher mistakenly @-ed you. I guess he wanted to @frederikprijck

🤔 It make sense hahaha

Narretz

Technically, the re-using of elements can also have an impact when using track by 'unique id'.
See https://plnkr.co/edit/24fcSM9h2AzgVvSt9ef4?p=preview

If you click "update" then you can see that for John 2, "fail" changes to "pass", but the directive is not re-compiled (the status color does not change) and the one-time binding is not updated.

track by $index only adds a special case: when you remove item 0, ngRepeat re-uses element 0, but with item 1's scope.
(If you use track by 'unique id', then ngRepeat re-uses element 1)

I wonder if we can make this clearer in the docs?

Narretz · 2017-11-23T16:34:26Z

src/ng/directive/ngRepeat.js

- * will not be updated even when the corresponding item changes, essentially causing the view to get
- * out-of-sync with the underlying data.
+ * {@link guide/expression#one-time-binding one-time bindings} or {@link guide/directive#creating-directives directives}.
+ * In such cases, where the array changes and `track by $index` is in use the `nth` DOM element


where ... -> when ..
in use the -> in use, the

Narretz · 2017-11-23T16:37:14Z

src/ng/directive/ngRepeat.js

+ * will always be matched with the `nth` item of the array. Since ngRepeat thinks that this item is the same as before,
+ * it will not re-create the DOM, but keep the existing one (bound to the existing scope), and therefore
+ * any directives that appear on the template will not be re-compiled (since compilation/linking happens only when a new instance is created).
+ * This is ngRepeats "keep track" function at play and it to reduce unnecessary DOM elements being rebuilt.


I don't think this last sentence adds useful info.

Narretz · 2017-11-23T16:38:24Z

src/ng/directive/ngRepeat.js

+ * In such cases, where the array changes and `track by $index` is in use the `nth` DOM element
+ * will always be matched with the `nth` item of the array. Since ngRepeat thinks that this item is the same as before,
+ * it will not re-create the DOM, but keep the existing one (bound to the existing scope), and therefore
+ * any directives that appear on the template will not be re-compiled (since compilation/linking happens only when a new instance is created).


Please keep this line at 100 characters

- deduplicate info between docs section and arguments - don't draw too much attention to track by ... - ... but highlight its drawbacks when used with one-time bindings - add example to show how tracking affects collection updates - clarify duplicates support for specific tracking expressions Closes angular#16332 Closes angular#16334

- deduplicate info between docs section and arguments - don't draw too much attention to track by $index ... - ... but highlight its drawbacks - add example to show how tracking affects collection updates - clarify duplicates support for specific tracking expressions Closes #16332 Closes #16334 Closes #16397

- deduplicate info between docs section and arguments - don't draw too much attention to track by $index ... - ... but highlight its drawbacks - add example to show how tracking affects collection updates - clarify duplicates support for specific tracking expressions Closes angular#16332 Closes angular#16334 Closes angular#16397

- deduplicate info between docs section and arguments - don't draw too much attention to track by $index ... - ... but highlight its drawbacks - add example to show how tracking affects collection updates - clarify duplicates support for specific tracking expressions Closes #16332 Closes #16334 Closes #16397

googlebot added the cla: no label Nov 20, 2017

googlebot added cla: yes and removed cla: no labels Nov 20, 2017

frederikprijck reviewed Nov 20, 2017

View reviewed changes

googlebot added cla: no and removed cla: yes labels Nov 20, 2017

docs(ngRepeat): update trackby $index usage

8e92a6d

Clarify the pitfalls of using `track by $index` Closes #16332

Narretz suggested changes Nov 23, 2017

View reviewed changes

Narretz mentioned this pull request Jan 9, 2018

docs(ngRepeat): improve info about tracking #16397

Merged

3 tasks

Narretz closed this in #16397 Jan 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updating track by $index usage #16334

Updating track by $index usage #16334

michael-letcher commented Nov 20, 2017

googlebot commented Nov 20, 2017

googlebot commented Nov 20, 2017

michael-letcher commented Nov 20, 2017

googlebot commented Nov 20, 2017

googlebot commented Nov 20, 2017

frederikprijck Nov 20, 2017 •

edited

Loading

michael-letcher Nov 20, 2017 •

edited

Loading

frederikprijck Nov 20, 2017 •

edited

Loading

michael-letcher Nov 20, 2017

frederikprijck Nov 20, 2017 •

edited

Loading

michael-letcher commented Nov 20, 2017

googlebot commented Nov 20, 2017

googlebot commented Nov 20, 2017

petebacondarwin commented Nov 20, 2017

frederikprijck commented Nov 20, 2017 •

edited

Loading

michael-letcher commented Nov 21, 2017 •

edited

Loading

petebacondarwin commented Nov 21, 2017

Narretz commented Nov 21, 2017 •

edited

Loading

frederikprijck commented Nov 21, 2017 •

edited

Loading

franciscovelez commented Nov 21, 2017

Narretz commented Nov 21, 2017

michael-letcher commented Nov 21, 2017 •

edited

Loading

franciscovelez commented Nov 22, 2017 •

edited

Loading

Narretz left a comment •

edited

Loading

Narretz Nov 23, 2017

Narretz Nov 23, 2017

Narretz Nov 23, 2017

Updating track by $index usage #16334

Updating track by $index usage #16334

Conversation

michael-letcher commented Nov 20, 2017

googlebot commented Nov 20, 2017

googlebot commented Nov 20, 2017

michael-letcher commented Nov 20, 2017

googlebot commented Nov 20, 2017

googlebot commented Nov 20, 2017

frederikprijck Nov 20, 2017 • edited Loading

Choose a reason for hiding this comment

michael-letcher Nov 20, 2017 • edited Loading

Choose a reason for hiding this comment

frederikprijck Nov 20, 2017 • edited Loading

Choose a reason for hiding this comment

michael-letcher Nov 20, 2017

Choose a reason for hiding this comment

frederikprijck Nov 20, 2017 • edited Loading

Choose a reason for hiding this comment

michael-letcher commented Nov 20, 2017

googlebot commented Nov 20, 2017

googlebot commented Nov 20, 2017

petebacondarwin commented Nov 20, 2017

frederikprijck commented Nov 20, 2017 • edited Loading

michael-letcher commented Nov 21, 2017 • edited Loading

petebacondarwin commented Nov 21, 2017

Narretz commented Nov 21, 2017 • edited Loading

frederikprijck commented Nov 21, 2017 • edited Loading

franciscovelez commented Nov 21, 2017

Narretz commented Nov 21, 2017

michael-letcher commented Nov 21, 2017 • edited Loading

franciscovelez commented Nov 22, 2017 • edited Loading

Narretz left a comment • edited Loading

Choose a reason for hiding this comment

Narretz Nov 23, 2017

Choose a reason for hiding this comment

Narretz Nov 23, 2017

Choose a reason for hiding this comment

Narretz Nov 23, 2017

Choose a reason for hiding this comment

frederikprijck Nov 20, 2017 •

edited

Loading

michael-letcher Nov 20, 2017 •

edited

Loading

frederikprijck Nov 20, 2017 •

edited

Loading

frederikprijck Nov 20, 2017 •

edited

Loading

frederikprijck commented Nov 20, 2017 •

edited

Loading

michael-letcher commented Nov 21, 2017 •

edited

Loading

Narretz commented Nov 21, 2017 •

edited

Loading

frederikprijck commented Nov 21, 2017 •

edited

Loading

michael-letcher commented Nov 21, 2017 •

edited

Loading

franciscovelez commented Nov 22, 2017 •

edited

Loading

Narretz left a comment •

edited

Loading