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
691 stars 157 forks source link

Computeds on encapsulated task's state not recomputing on change #424

Open validkeys opened 3 years ago

validkeys commented 3 years ago

I have an encapsulated task called "enqeueTask" (which I know are just plain objects now).

  @(task({
    reponse: null,
    dfd: null,

    respond() {
      return this.dfd.resolve(true)
    },

    *perform() {
      this.dfd = RSVP.defer()
      let response = yield this.dfd.promise
      this.response = response
      return this
    }
  }).enqueue()) enqueueTask

In the component that creates an instance of this encapsulated task I have a computed that returns the most recent task in the queue with "isRunning=true".

  @reads("enqueueTask.last") lastTask

  @computed("lastTask.isRunning")
  get currentTask() {
    const { lastTask } = this
    console.log("computing")
    return lastTask && lastTask.isRunning ? lastTask : null
  }

When that task completes, the computed property is not recomputing (even though the UI updates to show the task is complete). This was not a problem pre 2.0, so looking to find the 2.0 solution. What am I missing? Sorry if I'm being stupid!

I've created a Twiddle for reproduction here: https://ember-twiddle.com/5ce2f2282b79a2446cb61af2c5ee0e9c?openFiles=components.my-component%5C.js%2Ctemplates.components.my-component%5C.hbs

maxfierke commented 3 years ago

@validkeys I think using computed properties might be the issue. Using normal getters seems to work:

@task({ enqueue: true }) enqueueTask = {
  reponse: null,
  dfd: null,

  respond() {
    return this.dfd.resolve(true)
  },

  *perform() {
    this.dfd = RSVP.defer()
    let response = yield this.dfd.promise
    this.response = response
    return this
  }
}

get lastTask() { return this.enqueueTask.last; }

get currentTask() {
  const { lastTask } = this
  console.log("computing")
  return lastTask && lastTask.isRunning ? lastTask : null
}
validkeys commented 3 years ago

@maxfierke That certainly fixed it -- and confirmed my stupidity, I appreciate it!

maxfierke commented 3 years ago

To be clear -- this is may still be a bug, but I don't remember if @computed is supposed to work with Glimmer components or not

validkeys commented 3 years ago

I think it is a bug with encapsulated tasks. Computeds do work on glimmer components although aren't always necessary. There's definitely still some issues with the non-ember objects with encapsulated tasks (for example, waitForProperty in the below example will throw an error "Cannot redefine property: response"

    ...,
    response: null,
    defer: null,

    respond(answer) {
      if (typeof answer !== 'boolean') {
        throw new Error('respond only takes true/false as an argument')
      }
      this.response = answer
    },

    *perform(props = {}) {
      this.setProperties(props)
      yield waitForProperty(this, "response", v => !!v)
      return this.response
    }
adc-mhaugen commented 3 years ago

I may be experiencing the same issue with the lastValue decorator. I've just upgraded to 2.0.3 so I got rid of ember-concurrency-decorators. I'm using the lastValue decorator to cache the last value of a task, and I'm not seeing computed properties recompute when the lastValue changes. I installed ember-concurrency-decorators again and used the lastValue decorator from that package and it does update.

Techn1x commented 2 years ago

Just ran into this today as we dropped IE11 and upgraded to ember-concurrency 2 (I know, we're a little behind). I was hoping to be able to just get onto e-c 2 to get the benefits and worry about refactoring our large encapsulated tasks later, but we have a lot of computed properties watching the encapsulated tasks' state, so changing all of those into getters (and the implications of that) makes that work a much bigger job.

It's also a little tricky in combination with the waitForProperty bug, (where it doesn't update if waiting for a getter) https://github.com/machty/ember-concurrency/issues/292

miguelcobain commented 1 year ago

I also got bitten by this bug. Transitioning from ember-concurrency-decorators to just ember-concurrency is not seamless!

miguelcobain commented 1 year ago

I added a failing test for the @lastValue + computed case here: https://github.com/machty/ember-concurrency/pull/503

This failing test passes if I only export the computedLastValue here: https://github.com/machty/ember-concurrency/blob/master/addon/-private/task-decorators.js#L35

While doing this, I found out that using @dependentKeyCompat together with @lastValue does make this work. Example:

import { dependentKeyCompat } from '@ember/object/compat';

// ...

@dependentKeyCompat
@lastValue('task')
value;

So, I believe we have some options:

  1. document this problem and advise users to use @dependentKeyCompat
  2. somehow incorporate the @dependentKeyCompat into the decorator itself
  3. always use the computed version?

I guess version 2 would create be the most seamless migration path (as I think it is expected), but I also don't know how to implement it. For everyone else looking for a solution/workaround, you can go with option 1.

maxfierke commented 1 year ago

re: the issues raised w/ @lastValue:

This seems like it's working exactly as expected. However, we should have documented that @dependentKeyCompat is the expected tool to use in the case of computed & tracked interop, as it is elsewhere in Ember.

As e-c 2.x task state is all @tracked, this applies to the @lastValue decorator too, so @dependentKeyCompat is required if you still require that interop (e.g. versus converting your codebase's usage to native getters). Applying @dependentKeyCompat to @lastValue is not something without some cost, so prefer we not do that for those who're not using the computed property system.

If there's further discussion on @lastValue, I would ask that it be taken to Discord or a separate issue be opened, as it's really a separate issue than the one originally raised here having to do with encapsulated tasks.