machty / ember-concurrency

ember-concurrency is an Ember Addon that enables you to write concise, worry-free, cancelable, restartable, asynchronous tasks.
http://ember-concurrency.com
MIT License
690 stars 157 forks source link

Auto-tracked getter fails to recompute when yielding #343

Closed jgwhite closed 3 years ago

jgwhite commented 4 years ago

I’ve been struggling to figure out how to word this issue, so here’s a repro that I think demonstrates the problem:

http://jgwhite.co.uk/ember-concurrency-auto-tracking-repro/

Click the two “Run” buttons and you’ll see that when the task yields, auto-tracking seems to somehow break down.

maxfierke commented 4 years ago

I'll try to look at this more this week/weekend, but I'm not really sure how the auto-tracking stuff should work in this scenario. last is a computed alias for lastStarted on the scheduler, so it's possible it's just tracking assignments to last, but not updates to the value property within that, so it's reading the initial null for value because the task hasn't finished completing in the timeout case. (Again, not really sure how auto-tracking works, so this is all guessing.) It may work as expected with lastSuccessful or lastComplete, which should only be updated in the case of completion of the task, and thus value should be populated.

I'd also be curious to see how 2.0.0-alpha.0 works in this case, given it does far less using computed properties.

jgwhite commented 4 years ago

Thanks for looking into it. My understanding is not great either but I think the code should just work as written in the yield case. Not sure why the auto-tracking context is failing to capture the dependencies of the getter when yield is in the mix.

jgwhite commented 4 years ago

With 2.0.0-alpha.0 neither example works I’m afraid.

maxfierke commented 4 years ago

As a workaround, adding @computed('task.value.last') to the getter works, like so:

import Component from "@glimmer/component";
import { computed } from "@ember/object";
import { timeout } from "ember-concurrency";
import { task } from "ember-concurrency-decorators";

export default class AutoTrackingExampleComponent extends Component {
  constructor() {
    super(...arguments);
  }

  @computed("task.last.value")
  get result() {
    return this.task.last?.value;
  }

  @task
  *task(t = 0) {
    if (t) {
      yield timeout(t);
    }
    return Math.random();
  }
}

This may actually be the way to do this for now, as I think we'll run into the same problems as the auto-tracking-related bugs (#337, #340) if we just make those properties tracked on 3.15. However, we might be able to make tracked work with v2.

Edit: Using @computed also works for 2.0.0-alpha.0

jgwhite commented 4 years ago

Yeah, the more I look at this the more I think we’ll have to mark every property in e-c that’s ever set as tracked. For a second I thought I had a good grasp on why it was happening but very quickly it slipped through my fingers.

As a workaround, adding @computed('task.value.last') to the getter works… This may actually be the way to do this for now

This is reasonable, but the docs need to explain approximately why and when it’s necessary.

Anecdotally, folks on my team have taken to simply using @computed defensively with every getter since they started running into unintuitive issues with e-c and auto-tracking. This is a shame, but not e-c’s fault, perhaps more an inevitable pitfall of the transition from the classic Ember model to the Octane/Glimmer model.

maxfierke commented 4 years ago

Yeah, we should definitely document that. I'm not sure it's going to be possible to support both computed and tracked within v1 since its so dependent on computed properties. But for v2, I did start spiking on using tracked properties, so I'll see what's possible there.

jgwhite commented 4 years ago

I'm not sure it's going to be possible to support both computed and tracked within v1 since its so dependent on computed properties.

CPs and tracked properties are meant to be fully interoperable but I think we’re hitting something of an edge case here. I still feel like I really want to understand why the auto-tracking doesn’t work without this or the @computed change.

pzuraq commented 4 years ago

CPs and tracked properties are meant to be fully interoperable but I think we’re hitting something of an edge case here. I still feel like I really want to understand why the auto-tracking doesn’t work without this or the @computed change.

If task.value.last is a normal, non-decorated property, then autotracking will not work because it has no way to know that the property should be tracked, or will be updated at a later point. This is a known limitation of the autotracking interop, and the solutions are:

  1. Turn the property into a computed property
  2. Access the property with get() and update it with set()
  3. Release a new version of the library which drops support for older versions of Ember, and adopts @tracked for the property.

The eventual goal is for all libraries to move toward 3, but solution 1 can be used by library authors who want to maintain interop, and solution 2 can be used by users who are trying to interop with older libraries that have not yet been updated.

st-h commented 4 years ago

Just came across this issue after some parts of my app broke after upgrading to ember 3.17.0. The issue I was seeing is:

Cannot read property '_curry' of undefined
  at Object.compute (vendor-9457b666eed48599c63a5eac7dc6f382.js:formatted:50091)
  at r.o [as fn] (vendor-9457b666eed48599c63a5eac7dc6f382.js:formatted:2488)
  at vendor-9457b666eed48599c63a5eac7dc6f382.js:formatted:19149
  at e.track (vendor-9457b666eed48599c63a5eac7dc6f382.js:formatted:25076)
  at r.i.compute (vendor-9457b666eed48599c63a5eac7dc6f382.js:formatted:19148)
  at r (vendor-9457b666eed48599c63a5eac7dc6f382.js:formatted:19122)
  at new r (vendor-9457b666eed48599c63a5eac7dc6f382.js:formatted:2493)
  at vendor-9457b666eed48599c63a5eac7dc6f382.js:formatted:5193
  at Object.evaluate (vendor-9457b666eed48599c63a5eac7dc6f382.js:formatted:21113)
  at e.t.evaluate (vendor-9457b666eed48599c63a5eac7dc6f382.js:formatted:21038)

The workaround I found was to use a computed property and not access the task directly from a template (exactly as it has been described here). I am just a little surprised that more recent ember versions are mentioned here. 3.16.6 worked fine within my tests. 3.17.0 was the first version that fails. However, I might be seeing a different error - I haven't checked the error message of the reproducer mentioned here.

Interestingly, the error only showed up when I was referencing a property attached to the task from a progress element like <progress value={{task.version.upload.progress}}></progress>. If I change that to <div>{{task.version.upload.progress}}</div> everything runs fine. The computed property I used to fix looks like this:

progress: computed('task.version.upload.progress', function() {
    return this.task?.version?.upload?.progress;
})
st-h commented 4 years ago

My issue is very likely related to https://github.com/emberjs/ember.js/issues/18788 which is caused by invoking attribute={{task}}. Changing the variable name to something other than {{task}} seems to fix the issue as well.

maxfierke commented 3 years ago

This now works as expected in ^2.0.0-beta.1 on Ember 3.16+ as tracked properties are used underneath. Closing out.