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

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

Open
validkeys opened this issue Apr 27, 2021 · 9 comments
Open

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

validkeys opened this issue Apr 27, 2021 · 9 comments
Labels
bug ember-octane Issues related to use with Ember "Octane" Edition v2 Applies to ember-concurrency v2

Comments

@validkeys
Copy link

validkeys commented Apr 27, 2021

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
Copy link
Collaborator

@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
Copy link
Author

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

@maxfierke
Copy link
Collaborator

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
Copy link
Author

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
    }

@maxfierke maxfierke added bug ember-octane Issues related to use with Ember "Octane" Edition v2 Applies to ember-concurrency v2 labels Apr 29, 2021
@adc-mhaugen
Copy link

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
Copy link

Techn1x commented Jan 25, 2022

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) #292

@miguelcobain
Copy link

miguelcobain commented Nov 18, 2022

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

@miguelcobain
Copy link

I added a failing test for the @lastValue + computed case here: #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
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug ember-octane Issues related to use with Ember "Octane" Edition v2 Applies to ember-concurrency v2
Projects
None yet
Development

No branches or pull requests

5 participants