Improve CPs ergonomics by changing caching strategy

[cibernox]

Link to rendered

[stefanpenner]

i like this approach, upgrade path seems kinda rough though:(

[cibernox]

@stefanpenner I added a 4th idea involving decorators that can be a real option.

Having the @computed decorator have different semantics is not technically a breaking change because it's new syntax, so it could be doable and at the same time allow to deprecate old usages of computed.

[stefanpenner]

True

[rwjblue]

I personally like the idea that calling the setter would clear the cache and mark as dirty. I believe we can and likely should fix this within the 2.x cycle.

I propose the following path:

• Add a new feature flag to enable the new behavior.
• Allow that feature flag to be enabled at runtime throughout the 2.x cycle (value in features.json stays null throughout all of 2.x).
• Create a polyfill addon that can be used by apps and addons to use the new behavior on older Ember versions.
• After feature has hit 2.x.0-beta.1, add a deprecation when the feature is not enabled to canary (will become 2.(X+1).0-beta.1 after 6 weeks of deprecations).
• Enable the flag by default in newly generated ember-cli applications.
• Enable flag by default in 3.0.0.
• Remove old code.

Following the above path, we satisfy both our SemVer requirements and our requirement that 3.0.0 not have any non-deprecated changes.

[cibernox]

If I hear another positive voice from anyone else on the core I'll start a spike of the implementation.

[stefanpenner]

@rwjblue I'm unconvinced we can actually make this change in app CP as docs/guides/learning hazards would likely prevail not to mention addons that want to work in both worlds, now need to do special magic, but i do think we can use the decorators as a signal for a refresh.

If we can do the switch on a per CP basis, it could work though. This would also enable the required changes for decorators to opt-in.

[rwjblue]

FWIW, this exact path was followed by the ember-routing-drop-deprecated-action-style feature that removed looking up actions on the root of a controller. That flag was introduced around 1.5.0, and ultimately the deprecated code path was removed in 1.8.0.

---

@stefanpenner:

I'm unconvinced we can actually make this change in app CP as docs/guides/learning hazards would likely prevail

Our guides are already properly versioned, and our API docs are being actively worked on to support this versioning. I believe the current state is a much larger learning hazzard than providing a helpful deprecation + education process.

not to mention addons that want to work in both worlds

The majority of new features in the last few cycles have also implemented a nice polyfill-like addon that addon authors can use to get the new behavior across versions. This has been a pretty successful technique, and includes things like ember-new-computed addon (poorly named, but highly used), ember-debug-handlers-polyfill, and ember-getowner-polyfill.

I do think we can use the decorators as a signal for a refresh

Sure, I do not disagree, but I think that the path forward is fairly simple/straightforward as I laid out in #79 (comment).

[stefanpenner]

@rwjblue the polyfil approach, and enabling on a per computed property basis would address my concerns totally.

[rwjblue]

enabling on a per computed property basis would address my concerns

Ya, this is a great idea. Using the polyfill would already allow upgrading on a per CP basis, but we could also add an option flag to be used to force the setters return value to be cached (the existing/older behavior) even when the feature flag is enabled. Something like:

goldenRation: computed('height', {
  cacheSetterReturnValue: true,

  get() {
    return this.get('height') * 1.618;
  },
  set(_, value) {
    return this.set('height', value / 1.618);
  }

I would envision the feature flag would simply change the default value of cacheSetterReturnValue to false when enabled (which means it is still configurable if you really need/want the behavior).

[cibernox]

@stefanpenner The behaviour of the new CPs is more or less a subset of the original one.
Basically this removes caching from setters, and will most likely only affect people caching undefined.

I might be wrong, but seems compatible enough.

Also, what about something like

foo: computed('bar', 'baz', {
  get() { /* ... */ },
  set() { /* ... */ }
}).uncachedSetter();  // Enables the new behaviour when globally disabled

foo: computed('bar', 'baz', {
  get() { /* ... */ },
  set() { /* ... */ }
}).cachedSetter(); // Restores the old behaviour when globally enabled

This perhaps is pollyfillable.

[lukemelia]

I agree that the proposed approach would make the lessen the learning curve, though I'm quite sure there is a lot of code out there that relies on the current behavior.

If we decide not to change the behavior but want to improve user education, one idea would be to use the function.toString() approach mentioned in development builds to warn on setters that don't contain return.

[rwjblue]

@lukemelia - Yes, agreed, even if we do not want to change the go forward behavior (which I personally do want to change) we should make development ergonomics better.

[stefanpenner]

though I'm quite sure there is a lot of code out there that relies on the current behavior.

Yes, this. I know for a fact, large swaths of code depend on this (accidentally or intentionally). Many scenarios the get/set are not symmetric, and changes this would be quite painful.

Incrementally upgrading via new syntax and on a per-cp basis could mitigate these concerns. I would love to get @tomdale / @wycats input early though.

[workmanw]

I personally like the idea that calling the setter would clear the cache and mark as dirty. I believe we can and likely should fix this within the 2.x cycle.

I absolutely agree.

We have a few macros that have very complex CP logic for validating and marshalling data. In some cases we end up having to make a 3rd function that just calculates the return value so it's reusable for get and set. It's really frustrating to look at. This would be a welcomed changed.

[caseywatts]

This bit me again today lol. Here's my example in case it helps. I've actually been afraid to use get/set in the past because its behavior was always surprising to me - I suspect it's exactly because of this situation.

ES6 getter/setter docs suggest we don't use a return value during set(), and I thought Ember would behave the same.

I ended up with this silly code

  priceDollars: computed('priceCents', {
    get() {
      return centsToDollars(this.get('priceCents'));
    },
    set(attribute, value) {
      const priceCents = dollarsToCents(value);
      this.set('priceCents', priceCents);
      return centsToDollars(priceCents);
    }
  })

I also tried return this.get('priceDollars'); but that returns the old value lol

I'd rather have done this, with no return value:

  priceDollars: computed('priceCents', {
    get() {
      return centsToDollars(this.get('priceCents'));
    },
    set(attribute, value) {
      this.set('priceCents', dollarsToCents(value));
    }
  }),

[locks]

@caseywatts do you think https://guides.emberjs.com/v3.0.0/object-model/computed-properties/#toc_setting-computed-properties could be updated to make the behaviour clearer?

[caseywatts]

@locks good thinking!

• I could imagine a linter rule to cover You must return the new intended value of the computed property from the setter function. Just opened this. :)
• In the docs, we could explain what happens if you don't return the value. (I'm not sure what happens specifically, just that it caused bugs.)
• To really catch these edge cases (before bugs) I believe it might have to go as far as to suggest extracting a function like centsToDollars. That level of documentation for this feature would
  also probably make Ember look silly (versus fixing it).

[pzuraq]

We reviewed this RFC today in the core team meeting and realized that these are the semantics that we ended up adopting during the Decorators RFC. In classic usage, the old semantics still apply, but in native classes the new semantics take over.

Given it's been implemented, we decided to move this RFC into FCP to close. Thanks for working on it @cibernox!

[cibernox]

🙌🏻

[rwjblue]

Thank you for helping push this forward!