[BUGFIX beta] Add warning for “deep @each” usage in dependent keys

[jgwhite]

Adds a warning when defining dependent keys of the form todos.@each.owner.name or todos.@each.owner.@each.name as described in the guides.

Fixes #12840.

[stefanpenner]

r? @rwjblue @krisselden

[rwjblue]

LGTM

[rwjblue]

Can you rebase?

[homu]

☔ The latest upstream changes (presumably #12848) made this pull request unmergeable. Please resolve the merge conflicts.

[krisselden]

should it not be an assertion? also, should we say the workaround is to create an intermediary computed property?

[mmun]

If its an assertion it may break people's apps who are using it today without knowing that it doesn't work. 👍 for suggesting an intermediate CP in the meantime.

[krisselden]

Other than I think we should consider it an error since it doesn't actually work to do this, this looks good to me.

[stefanpenner]

I don't think it can be an error until it's deprecated, etc. even though it doesn't work as expected. Would cause mega emo for little win.

[jgwhite]

Rebased and applied @krisselden and @mmun suggestions.

[rwjblue]

Thanks @jgwhite!

[jgwhite]

<3

[barneywilliams]

Thanks much for this patch! It has cost us many hours and bugs, and we are still concerned that it would be popping up from time to time.

Although, I think that an error would be more appropriate than a warning... since it doesn't even work. But this is great for starters...

The user base would MUCH appreciate if these dependency variants could actually be supported. Is this being considered?

[stefanpenner]

@barneywilliams it would require a champion, I don't believe anyone currently is. Maybe someday

[barneywilliams]

@stefanpenner Fully understand and agree. Maybe I can carve out the motivation to investigate and maybe take a stab at it some night or weekend...

[rwjblue]

FWIW, I think we all generally agree that we would like foo.@each.bar.baz to work and just haven't had the time to tackle it.

I'm not really sold on making foo.@each.foo.@each.baz work though...

[stefanpenner]

I'm not really sold on making foo.@each.foo.@each.baz work though...

Ya, its possible but ends up being quite costly to maintain. Maybe after/while the next push on chains/references happens.

[barneywilliams]

Thanks for the feedback guys. I have another co-worker/ember-dev that is willing to pitch in as well, so maybe we can actually make this happen.

I agree/understand the additional complexities with nested @eaches that may be difficult to maintain.

I am going to file a request and link back to this post (for reference). Maybe we can drum up more support there as well.

[cibernox]

This will warn also on hidden usages of this like computed.filterBy("array","state.isActive",true)?

[jgwhite]

@cibernox looks like the array macros use computed(dependentKey, ...) so yeah, should be good.

[jgwhite]

This patch also warns for dep keys of the form foos.@each.bars.[]. I take it this case is also unsupported?

/cc @stefanpenner @rwjblue

[runspired]

@barneywilliams all of those situations are accomplishable with two computed properties. It's possible if you need it enough to write a computed property of your own that generates the necessary first step computed for you. Not sure how well it would perform, but would cut down some boiler plate from time to time.

[barneywilliams]

Chris, that is a good thing to point out on this thread. That coupled with the new warning is definitely buttons it all up better and may be a decent place to settle.

Aesthetically, it bugs me to write CPs that are only used internally, especially to work around limitations.

Although, Ember is very powerful as it is and am glad there are solutions, albeit workarounds, for these limitations.

On Feb 4, 2016, at 5:33 PM, Chris Thoburn notifications@github.com wrote:

@barneywilliams all of those situations are accomplishable with two computed properties. It's possible if you need it enough to write a computed property of your own that generates the necessary first step computed for you. Not sure how well it would perform, but would cut down some boiler plate from time to time.

—
Reply to this email directly or view it on GitHub.

[Serabe]

Actually interested on the last case @jgwhite explained. I have a hierarchy of classes and I use some CP's coming from a flat-map operation.

I have a class A that has many class B that has many class C. Then:

const C = Ember.Object.extend({
  prop: Ember.computed(/* something that resolves to an array */)
});

const B = Ember.Object.extend({
  cs: collectionOfInstancesOfC,
  prop: Ember.computed(
    'cs.@each.prop.[]',
    function() { flatMap('cs', 'prop'); }),
});

const A = Ember.Object.extend({
  bs: collectionOfInstancesOfB,
  prop: Ember.computed(
    'bs.@each.prop.[]',
    function() { flatMap('bs', 'prop'); }),
});

Which would be the right way to do this to avoid the warning?