Avoid "update prop already used in computation" #802
Conversation
Ember internals use an observer for attribute bindings. In this case the observer timing causes a computation based on a value which is then updated by a side-effect-having CP. Here avoid the observer interaction by instead setting the count itself as part of the computation side effects. See: * Addepar#795 * emberjs/ember.js#18613 Fixes Addepar#795
| @@ -306,6 +309,11 @@ export default Component.extend({ | |||
| this.collapseTree.set('rowMetaCache', this.rowMetaCache); | |||
| this.collapseTree.set('rows', rows); | |||
|
|
|||
| run.schedule('actions', () => { | |||
| // eslint-disable-next-line ember-best-practices/no-side-effect-cp | |||
| this.set('dataTestRowCount', this.get('wrappedRows.length')); // eslint-disable-line ember/no-side-effects | |||
There was a problem hiding this comment.
do we need two eslint-disable comments here?
There was a problem hiding this comment.
There are indeed two rules which error on this.
It is actually a bit annoying since the side effects really begin on line 309, but I presume the lint only catches on this.set
| @@ -306,6 +309,11 @@ export default Component.extend({ | |||
| this.collapseTree.set('rowMetaCache', this.rowMetaCache); | |||
| this.collapseTree.set('rows', rows); | |||
|
|
|||
| run.schedule('actions', () => { | |||
| // eslint-disable-next-line ember-best-practices/no-side-effect-cp | |||
| this.set('dataTestRowCount', this.get('wrappedRows.length')); // eslint-disable-line ember/no-side-effects | |||
There was a problem hiding this comment.
if this passes the test then LGTM but obviously this super sucks explicitly adding this sideeffectyness. Thankfully it's pretty limited side effect but still 🤢
There was a problem hiding this comment.
Right, as noted this is expanding on an already-existing side effect which caused the problem. An alternative here would be to simply stop offering the data-tests-row-count attribute. If we drop 1.12 then this would be rewritten using didReceiveAttrs and the count could be set there without any CP side effects.
|
I believe the remaining failures are un-related to anything in ember-table, instead they should be resolved by ember-cli/ember-cli-htmlbars#423 landing and getting released. |
mixonic
force-pushed
the
mixonic/3.15-compat
branch
from
b63842b to
e939236
Compare
Avoids the issue with Node 8 caused by npm/node-semver@d61f828#diff-b9cfc7f2cdf78a7f4b91a753d10865a2R35
mixonic
force-pushed
the
mixonic/3.15-compat
branch
from
e939236 to
d923e47
Compare
|
It is green! I had to add an additional commit to ignore engines wrt: npm/node-semver@d61f828#diff-b9cfc7f2cdf78a7f4b91a753d10865a2R35 This change makes the tests slightly more brittle but we don't need to stop testing with Node 8. Given that this addon nominally supports Node 8 that seems like the right tradeoff. |
Ember internals use an observer for attribute bindings. In this case the observer timing causes a computation based on a value which is then updated by a side-effect-having CP.
Here avoid the observer interaction by instead setting the count itself as part of the computation side effects.
See:
Fixes #795