Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] Ember octane computed properties regression #19279

Closed
lifeart opened this issue Nov 18, 2020 · 6 comments
Closed

[Bug] Ember octane computed properties regression #19279

lifeart opened this issue Nov 18, 2020 · 6 comments

Comments

@lifeart
Copy link
Contributor

lifeart commented Nov 18, 2020

Minimal reproduction: https://ember-twiddle.com/ed259f439203096ac88c0e0aaa3383f1?openFiles=twiddle%5C.json%2C

Working in 3.12
Not working in 3.13 (from beta) till 3.23

Tl dr:
nested computed properties, related to arrays not triggered

This issue does not related to ember-data, tested on (3.12 - 3.23), and only ember-source cause regression


One more behavour (likely not related to this bug, but may be related to computed properties)
during an large MDD app migration to Octane, I've faced to issue, related to relation tracking, using computed properties - https://ember-twiddle.com/ed17ef8abcdfca94599b2e5aa385485d?openFiles=templates.application%5C.hbs%2C
(middlename value appear in input, but not appear in template after new model was added)

Without e-data it seems working as expected https://ember-twiddle.com/222a3f1e1bb9f244b35a725fc03621f7?openFiles=templates.application%5C.hbs%2C

@lifeart
Copy link
Contributor Author

lifeart commented Nov 18, 2020

@lolmaus
Copy link
Contributor

lolmaus commented Nov 18, 2020

simplier regression case

Your CP allAdditives depends on metaAttributes.@each.countAdditives but is not using it. Instead it uses metaAttributes.@each.additives.[] which Ember has been known not to support.

When I rewrite your CP to use metaAttributes.@each.countAdditives it does recompute correctly.

Can you update your reproduction such that CPs use exactly the properties they depend on and don't access two arrays deep in one CP?

@lifeart
Copy link
Contributor Author

lifeart commented Nov 18, 2020

@lolmaus countAdditives is alias to additives.length, and it should invalidate computed property because I'm pushing values to additives array, and this is behavour is working fine just before octane

@lolmaus
Copy link
Contributor

lolmaus commented Nov 18, 2020

AFAIK:

  1. Ember documentation says somewhere that a CP can not depend on a property two levels deep into arrays.
  2. Ember documetnation says somewhere that if a CP uses a property in its code then this property should be listed in that CP's dependencies. It's kinda wront to depend on A but use B.

But I was able to reproduce this bug while adhering to these two points!

Repro (set to 3.13.1, works with 3.12.1):
https://ember-twiddle.com/f1c5b4ecb5c4eed81c729559cdfe63cc?numColumns=2&openFiles=controllers.application\.js%2Ctemplates.application\.hbs

  // still need `countAdditives` to trigger recomputation
  allAdditiveArrays: computed('metaAttributes.@each.{countAdditives,additives}', function () {
    return this.metaAttributes.map((meta) => meta.additives);
  }),

  allAdditives: computed('allAdditiveArrays.[]', function(){
    return [].concat.apply([], this.allAdditiveArrays); // flatten array of arrays
  }),

@lolmaus
Copy link
Contributor

lolmaus commented Nov 18, 2020

Oh, I just forgot how to do it properly. The flattening CP should depend on arrayOfArrays.@each.[]. 😎

This works in 3.13.1:

  allAdditiveArrays: computed('metaAttributes.@each.additives', function () {
    return this.metaAttributes.map((meta) => meta.additives);
  }),
  
  allAdditives: computed('allAdditiveArrays.@each.[]', function(){
    return [].concat.apply([], this.allAdditiveArrays);
  }),

https://ember-twiddle.com/eff8e81ab024dc8bcf5cc39d833c7d66?numColumns=2&openFiles=controllers.application%5C.js%2Ctemplates.application%5C.hbs

rwjblue added a commit that referenced this issue Dec 15, 2020
…nested-alias-octane-regression

[BUGFIX lts] Ensure aliases are bootstrapped when using @each, #19279
@lifeart
Copy link
Contributor Author

lifeart commented Jan 25, 2021

closing it, because fix is merged to 3.24

@lifeart lifeart closed this as completed Jan 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants