isRunning and isIdle are being changed twice within one render while using #each loop within the check

[themmer]

Let me know if you have any ?'s. This behavior is happening when I upgrade to 0.8.0 version of ember-concurrency. This was working in the 0.7.x version. My goal is to side load data (lazy load) and then display the list of data. Here is the setup code and error:

*.js

  songs: computed('album', {
    get() {
      const album = this.get('album');
      return this.get('songsAlbumTask').perform(album.get('id'));
    }
  }),

  songsAlbumTask: task(function*(albumId) {
    return yield this.get('store').query('song', { albumId });
  }).drop(),

*.hbs

{{#if songsAlbumTask.isRunning}}
  {{spinner-display}}
{{else}}
  {{#each songs as |song|}}
    Doesn't matter what you put here
  {{/each}}
{{/if}}

If I take the each loop out of the template there is no error below

Assertion Failed: You modified "songsAlbumTask.isRunning" twice on <Task:songsAlbumTask> in a single render. It was rendered in "component:album-view" and modified in "component:album-view". This was unreliable and slow in Ember 1.x and is no longer supported. See https://github.com/emberjs/ember.js/issues/13948 for more details.
Error
    at assert (http://localhost:4200/assets/vendor.js:17191:13)
    at assert (http://localhost:4200/assets/vendor.js:28942:34)
    at Object.assertNotRendered (http://localhost:4200/assets/vendor.js:34717:11)
    at propertyDidChange (http://localhost:4200/assets/vendor.js:33333:30)
    at http://localhost:4200/assets/vendor.js:33411:7
    at Meta._forEachIn (http://localhost:4200/assets/vendor.js:31311:11)
    at Meta.forEachInDeps (http://localhost:4200/assets/vendor.js:31281:19)
    at iterDeps (http://localhost:4200/assets/vendor.js:33399:10)
    at dependentKeysDidChange (http://localhost:4200/assets/vendor.js:33375:7)
    at Object.propertyDidChange (http://localhost:4200/assets/vendor.js:33315:9)

[alexander-alvarez]

does this have anything to do with it?

{{#if songsAlbumTask.isRunning}}
  {{spinner-display}}
{{else}}
  {{#each songs.value as |song|}} {{!-- songs.value instead of songs --}}
    Doesn't matter what you put here
  {{/each}}
{{/if}}

[Sinled]

Also have this problem, with 0.7.19 all work fine

[themmer]

@alexander-alvarez I will have to check that out when I revisit the version upgrade, thanks!

[leondmello]

I was facing this issue as well, but then I had a close look at my code. It was the behavior of my app as I was triggering the task on a CP get. I revisited my code and changed it to run on the afterRender queue of the run loop.

[themmer]

I updated to .8.7 version to see if anything changed, but seeing the same thing.

@alexander-alvarez I was unable to fix the problem with your suggestion and also tried the await helper. I appreciate the help!

@leondmello I tried adding a run after render scheduler inside the computed property as you mentioned in a simpler scenario to make things more straight forward.

Did you do anything different than my sample below? This did fix the rendering twice problem. I didn't know if that was a side effect with the new refactor of the .8 version.

Inside a computed property used in the template to pass down to another component:

gems (computed property):

      return new RSVP.Promise(resolve => {
        Ember.run.schedule("afterRender", this, function () {
          resolve(this.get('gemsTask').perform(category.get('id')));
        });
      });

So the above fixes my problem and I can go that route assuming this is the new direction. I'm thinking this seems like more of a workaround than a solution maybe.... ?

Thanks everyone!

[machty]

I'm not sure how to classify these kinds of bugs. It seems like fundamentally the issue is this:

{{#if someValueThatStartsOffAsFalse}}
  ...
{{else}}
  {{someComputedPropertyThatChangesTheAboveValueToTrue}}
{{/if}}

Within a single render, you're kicking off code that changes bound values that had already rendered. This is kind of why I'm personally against the pattern of kicking off code via implicity .get()s in the template, but I guess in some cases it's unavoidable.

Perhaps one alternative here is to use .on('init') on the task which will kick it off on component initialization rather than waiting for it to be rendered from within the template.

[sukima]

In my run ins with this problem it was always because my code was violating the Single Responsibility Principle. In other words my task was responsible for returning a value to populate (read satisfy) the getter and set something as a side effect of getting something. These two should be separate actions.

Performing setup or populating values should be in reaction to say didInsertElement while the use of this.get(...) or use as a value in a template should not the the trigger for doing something else. Kind of like how we tout Data Down; Actions Up to prevent strange side effects of two way binding. In this case we have a task that is performed because we happen to attempt to use it's (soon to be) value in the template.

I think this is a case where a PromiseProxyMixin is a better solution then an e-c task. Or alternatively make your templates fault tolerant and don't toggle it based on a running task.

{{#if myTask.isRunning}}
  Loading...
{{/if}}
{{#each myTask.last.value as |something|}}
  Will render only when myTask.last.value is not null;
  which it is while myTask.isRunning is true.
{{/each}}

didInsertElement() {
  this._super(...arguments);
  this.get('myTask').perform()
}

[theseyi]

@sukima @machty Given the above, what's the best approach for the use case, where the Task is intended to be triggered when a dependent key / computed property changes? using the original example as a reference, the "album" is changed, and the related songs need to be updated

from above:

  songs: computed('album', {
    get() {
      const album = this.get('album');
      return this.get('songsAlbumTask').perform(album.get('id'));
    }
  }),

  songsAlbumTask: task(function*(albumId) {
    return yield this.get('store').query('song', { albumId });
  }).drop(),

didInsertElement will work for the initial render, but subsequently, if for example, the user changes the selected album, this will fail

[skwongr]

(This probably not resolving the issue but just an idea) Ember-Concurrency is great to use. But in the "album" case might not be necessary, it could be like:

  songs: computed('album.id', () => {
    let props = this.getProperties('store', 'album.id');
    return props.store.query('song', { props['album.id'] });
  }),

And if you want to show the loading state, the hbs would like

{{#if songs.isPending}}
  {{spinner-display}}
{{else}}
  {{#each songs as |song|}}
    ....
  {{/each}}
{{/if}}

Just my 2 cents

[theseyi]

@skwongr Thanks. Yes, e-c is never necessary, you can always implement your solution, but one of the benefits is that when dealing with async state, such as in the album example, you can take advantage of derived state like isRunning, isIdle, e.t.c and not have to wire up your own isPending state flag if you're not using something like ember-data. The album example is just an example, there are many similar scenarios where this is useful.

[sukima]

@theseyi I would reverse the process:

songsAlbumTask: task(function*() {
  let albumId = this.get('album.id');
  let song = yield this.get('store').query('song', { albumId });
  this.set('song', song);
})
.drop()
.observes('album.id'),

[theseyi]

I see, with an observer rather than a computed property. I guess, given Alex's comment above, CP's are not recommended approach. thanks @sukima

[sukima]

@theseyi Yeah CP are about fetching and adding side effects in them is glutton for punishment. Since most CP are synchronous this is typically never an issue. For example if I asked for this.get('nickname') I would never expect something else to change. I would not expect a this.set in there. But with async you have to return something and the real value suddenly apears later ephemerally.

Vanilla Ember (and by proxy ember-data) handles this by providing an abstraction around the async code called PromiseProxyMixin (A.K.A. PromiseObject in ember-data) which acts as a value which ephemerally becomes something later on. Because ember knows how to deal with data changes this all fist just fine.

ember-concurency does something similar by abstracting the async code in a TaskInstance which is much like a PromiseObject. Hence the derived state. However it's API is designed to be performed as an action not as a lookup of a value and so can become quite confusing in terms of side effects in getters.

I found that it is much easier to reason about when the task is treated as an action which would set state within the task instead of attempting to use the act of getting to perform a task. It is the difference between asking for a value and telling a task to do something.

I hope that clarifies things a bit.

[theseyi]

Thanks @sukima