Computed Properties broken with @each in 2.1.0

[ameliadowns]

Every computed property based on something using @each has broken with the 2.1.0 upgrade. Here are two examples:

  titleUniqueInCollection: Ember.computed('collection.sections.@each.title', function() {
    const sections = this.get('collection.sections');
    const sectionsWithSameTitle = sections.filterBy('title', this.get('title'));
    return sectionsWithSameTitle.length === 1;
  }),

  sortedCollectionItems: Ember.computed('collectionItems', 'collectionItems.@each.rankPosition', function() {
    return this.get('collectionItems').sortBy('rankPosition');
  }),

The functions are no longer running as expected when an attribute is changed.
Edited to reflect @stefanpenner's comment.

[stefanpenner]

can you provide a jsbin or similar to demonstrate the issue in isolation.

Also some little things, hopefully to prevent future confusion:

• these aren't observers just computed properties
• they don't fire, rather in two in they will be marked as dirty, and if someone is interested they will consume (but only if someone like an active template chooses to)

[e00dan]

I wonder if this is related to #12328.

[ameliadowns]

Here's a JS Bin.

I added a broken computed property to @kuzirashi's JS Bin from #12328

[stefanpenner]

may be related to -> #11983 / #11984 / #11983 cc @krisselden

[adam-knights]

Is this likely to be a 2.1 patch or should we plan to wait for 2.2?

[g-cassie]

Also looking for guidance on here as to whether this will get patched or if we are looking at a fix for 2.2. If there is anything that a novice can do to help move things along please let me know.

[rwjblue]

@g-cassie - I haven't had time to really dig into this, but if a fix is proposed it will definitely be in 2.2 and depending on how invasive it is we could backport to 2.0/2.1 as needed. The first step is that someone needs to figure it out and fix the bug.

[davidsteinberger]

@ameliadowns @g-cassie I'm facing the same issue. Has either of you found a way to get the CP listening to @each.property_name to recompute?

[g-cassie]

@davidsteinberger I just reverted to Ember 2.0 for the time being. The problem seems to be isolated to 2.1

[krisselden]

@stefanpenner have you or anyone on this thread confirmed that one of those PRs mentioned caused this issue? I'll try to get to this soonish, it would help to have a failing test PR if anyone interested in this issue wants to see it addressed sooner.

[g-cassie]

@krisselden I think #12538 is a failing test PR for this issue.

[ameliadowns]

@davidsteinberger, I did the same as @g-cassie and reverted to Ember 2.0.

[stefanpenner]

@krisselden no i have not yet had a chance.

[stevesims]

+1 from me
We're seeing this issue in our codebase too - we've got a CP that has a dependent key of record.relatedItem.@each.hasDirtyAttributes which never updates. even when one of the relatedItem records definitely becomes dirty.
I suspect we'll need to revert to Ember 2.0 as well to work around this

[adam-knights]

@g-cassie I took a look at your test and I think it fails in Ember 2.0 too so not sure it shows this issue.

[andorov]

I have seen an issue with an array observer ('[]' and not '@each', but possibly the same root cause) not triggering in Ember 2.0.0

[stevesims]

Further to my earlier note, as per emberjs/data#3911 we've worked around by using a dependent key of record.relatedItem.content.@each.hasDirtyAttributes, so fortunately we haven't had to revert to 2.0
Would still be very nice to see this fixed.

[adam-knights]

I tried Ember 2.2.0 today and we still see issues with @ each.

[toovy]

I can confirm that this is still not working in v2.3.0-beta.2.

[adam-knights]

I've played around with some of the unit tests at various levels and I have yet to find something that passes in ember 2.0 and fails in 2.1/2.2, including the pull request above. Has anyone been able to see this regression with non ember data objects?

[csantero]

@adam-knights I believe this has been broken since at least 2.0. I tested a version of that PR against each major release and the last one it was working on was 1.13.

[adam-knights]

@csantero I'd love to do some work to help identify the exact commit that did this but my issue was getting your PR to pass on 1.13, I never got it passing, did you modify it in some way?

[csantero]

@adam-knights IIRC the file with related tests moved between then and now, but otherwise the test was the same.

[YoranBrondsema]

FWIW we're seeing the same issue which prevents us from upgrading to Ember 2.1.

[nybblr]

Same issue on a project I'm running w/ Ember 2.2, happens when observing through PromiseManyArrays.

Interestingly, if I render a property in a template (forcing an access) within the .@each, it forces a recompute I guess and observation works.

[stefanpenner]

Sorry guys, the last little while has been hectic. I'll try to carve out some time in the next week or so (feel free to ping me If i don't respond again in a week, that means I just forgot).

[jeradg]

@stefanpenner You're a champ.

[ef4]

Folks who are watching this issue and interested in exploring the problem should check out the test cases in #12832

It looks like there is at least one longstanding bug in ember-metal that's contributing to this problem, and it was revealed by more recent changes.

[stefanpenner]

@jgwhite that is not a supported behavior, you can only go one leaf beyond the @each

@runspired / @jgwhite we should make this a warning. It is a trap, and seemed to work, due to performance bug.

[jgwhite]

@stefanpenner good idea. Submitted my attempt in #12847.

[mmun]

Hey @fivetanley, I had a similar bug in my app that I was able to bisect down to the same commit. With the help of @krisselden, I believe the issue is that the offending PR changed content to be a computed property, but this computed property is clobbered by https://github.com/emberjs/data/blob/c8f03a5c1c7b91160e6c3ad51b80638efd1b0413/addon/-private/system/record-arrays/record-array.js#L44. It may also be clobbered in other places. I haven't looked closely yet.

We should first try to rewrite the PR in a way that does not use a CP (and back port fixes to 2.1, 2.2 and 2.3) so that other consumers of ArrayProxy don't have the same issues. If that strategy fails we can change ember-data to not clobber instead.

[gabrielrotbart]

Thanks a lot @mmun. Really appreciated, even though the solution is not ideal

[YoranBrondsema]

@mmun Thanks for fixing! One question though: as far as I can see, the bugfix #12908 has only been released for 2.3 and 2.4. However, this bug is preventing us from upgrading from 2.0 to 2.1. We would like to avoid having to upgrade straight to 2.3 from 2.0. Would you be able to do a bugfix release for 2.1 and 2.2 as well? Or has the code diverged too much and is causing many conflicts.

[rwjblue]

We generally do not backport bugfixes to all affected versions. 2.0/2.1/2.2/2.3 are all compatible versions, I am not aware of any breaking changes amongst them.

[stefanpenner]

Or has the code diverged too much and is causing many conflicts.

Its just a time thing, maybe someday we will have more of it 💀

[hoIIer]

just ran into this on ED 3.0.2 where:

    productCount: computed('cart_lines.@each.quantity', function() {

if triggered upon line item create but not upon quantity update of existing. however this:

    productCount: computed('cart_lines.content.@each.quantity', function() {

does fire when it updates... is this supposed to still be happening in the latest Ember Data?

[mmun]

@erichonkanen The bug that you're describing is another bug that has been fixed on ember-data master.