[CLEANUP] Don't cache length property in for loops #12744
Conversation
|
I'm not sure if the failed tests are due to this change or some weird IE9 behavior. It seems to timeout in two tests which seem unrelated to these changes. |
|
I will try to review in more detail later tonight/tomorrow, but one thing I wanted to make sure you took into account was that sometimes caching the length is actually needed. Specifically, when you are in a loop and you are mutating that array inside the loop you may need to cache |
| @@ -40,7 +40,7 @@ ChainWatchers.prototype = { | |||
| remove(key, node) { | |||
| let nodes = this.chains[key]; | |||
| if (nodes) { | |||
| for (var i = 0, l = nodes.length; i < l; i++) { | |||
| for (var i = 0; i < nodes.length; i++) { | |||
| if (nodes[i] === node) { | |||
| nodes.splice(i, 1); | |||
There was a problem hiding this comment.
Self note: This loop length should probably be cached.
There was a problem hiding this comment.
On a second look, this shouldn't need to cache length as we break the loop on the next line, might be confusing though.
|
I did review surrounding code and it seemed that all those loops never mutated the iterated array / array-like object. Unless there are some liverecord-like arrays involved, there shouldn't be any issues. I might missed one or two, currently double-checking the whole commit myself. |
|
☔ The latest upstream changes (presumably #12831) made this pull request unmergeable. Please resolve the merge conflicts. |
topaxi
force-pushed
the
dont-cache-length
branch
from
b3bf9a0 to
7082e58
Compare
|
Resolved merge conflicts and rebased on master. |
|
Seems good, we will just need to double check each change. |
|
☔ The latest upstream changes (presumably b8786ba) made this pull request unmergeable. Please resolve the merge conflicts. |
|
☔ The latest upstream changes (presumably e07d3cf) made this pull request unmergeable. Please resolve the merge conflicts. |
topaxi
force-pushed
the
dont-cache-length
branch
from
7082e58 to
bc1edcf
Compare
|
☔ The latest upstream changes (presumably #12929) made this pull request unmergeable. Please resolve the merge conflicts. |
topaxi
force-pushed
the
dont-cache-length
branch
from
bc1edcf to
c482419
Compare
|
☔ The latest upstream changes (presumably #12928) made this pull request unmergeable. Please resolve the merge conflicts. |
topaxi
force-pushed
the
dont-cache-length
branch
from
c482419 to
af25048
Compare
topaxi
force-pushed
the
dont-cache-length
branch
from
af25048 to
b2c9e1c
Compare
|
☔ The latest upstream changes (presumably b295edb) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Thank you for your time on this! I'm going to go ahead and close this for now as it seems to have gone stale. I'm happy to reopen if you would like to pick this back up again. |
|
Rebased and pushed to the branch https://github.com/topaxi/ember.js/tree/dont-cache-length I'm happy to reopen a new PR and update or rebase this as needed, just ping me if new merge conflicts happen. |
|
Weird, it won't let me reopen the PR (I was able to reopen at least one that I closed yesterday). |
Just like in emberjs/data#4015