[PERF beta] @each should remain a stable node for chains.
#11990
Conversation
`@each` was designed for chaining, the special cased eager behavior + it invalidating for array changes meant that the EachProxy and chains were rebuilt every array change, instead of the leaves just changing. Conflicts: packages/ember-runtime/lib/system/each_proxy.js packages/ember-runtime/tests/mixins/array_test.js
|
|
||
| if (lim>0) { removeObserverForContentKey(content, key, this, idx, lim); } | ||
|
|
||
| let keys = this._keys; |
There was a problem hiding this comment.
is this._keys ever legitimately contain prototype extensions? If not, maybe we should make it an EmptyObject. This would allow us to also skip the bellow !keys.hasOwnProperty(key)
There was a problem hiding this comment.
a here it is: https://github.com/emberjs/ember.js/blob/fix-each-beta/packages/ember-runtime/lib/system/each_proxy.js#L79
ya this should be one of those EmptyObjects and we can take the fast-path for x in y without HOP.
There was a problem hiding this comment.
It is a empty, this is beta though
|
i believe this needs a deprecation in 1.13.x (and it is worth any tiny pain users may have with an additional deprecation. |
| @@ -22,7 +22,7 @@ var SPLIT_REGEX = /\{|\}/; | |||
| Ember.expandProperties('{foo,bar}', echo); //=> 'foo', 'bar' | |||
| Ember.expandProperties('foo.{bar,baz}', echo); //=> 'foo.bar', 'foo.baz' | |||
| Ember.expandProperties('{foo,bar}.baz', echo); //=> 'foo.baz', 'bar.baz' | |||
| Ember.expandProperties('foo.{bar,baz}.@each', echo) //=> 'foo.bar.@each', 'foo.baz.@each' | |||
| Ember.expandProperties('foo.{bar,baz}.[]', echo) //=> 'foo.bar.[]', 'foo.baz.[]' | |||
|
I just want to make sure I'm clear on this cause the wording of the description is a bit confusing. Say I have isOvertime: Ember.computed('shifts.@each.hours', function() {
var total = this.get('shifts').reduce(function (sum, shift) {
return sum + shift.get('hours');
}, 0);
return total > 40;
});Would |
|
@raytiley ok, I do need to make it more clear, this is only targeting where people used |
|
gotcha... so doing |
|
@raytiley - Confirm |
|
Merging this into beta. I am working on some deprecations specifically for stable that will ensure |
[PERF beta] `@each` should remain a stable node for chains.
|
This change should be probably mentioned clearly in the next beta release notes. There must be few people like me who have made the switch to 2.0 beta already and won't see the deprecation. Still a really minor thing, those who use the very latest versions probably follow PRs too. |
|
@rwjblue based on ^ should we leave some hints/warnings/asserts/errors in 2.x? Might be a common thing, especially as people who are knew to the concept. We can likely catch the mistake extremely early and instruct the correct usage. |
|
@stefanpenner / @ilkkao - Helpful assertion added in #12019. |
|
cool, thanks. |
@eachwas designed for chaining, the special cased eager behavior + it invalidating for array changes meant that the EachProxy and chains were rebuilt every array change, instead of the leaves just changing.