How to do you check for `isDestroyed`?

I still think components should not make AJAX calls. These really should be put in a service that will continue to exist after the component is destroyed.

I firmly disagree. The root of why is that services are singletons and components are not. When you have more than one component of the same type, IE lists, and they need to get extra data or make a server call based on user action, that resulting state has to live somewhere. That is the heart of this problem.

The actual $.ajax call can live in a service, that’s fine. But when the AJAX promise is fulfilled and the resulting state is returned, it has to live somewhere. If it lives on the service, it would have to do so in an Array with other similar data. If that’s the case, then every component has to scour through a list of data and map it back into it’s UI. That is likely to cause more performance issues, not less.

Here is a real world use case from our app. We have a list of entities, call them ResearchItem. We display 50 research items at a time in a collapsed fashion. The user can expand them to see more details and we go to the server to fetch that data. We actually have a service to do this, the function getMoreDetails(researchItem) returns a promise. Our list components register handlers on the promise and store the data on the component when it resolves.

I think that there are probably many apps that are more simplistic in nature, such as form software, and don’t require multiple AJAX calls to fill a page with data. And for them components not making AJAX calls is probably fine. But you can’t always assume that.

Very good implementation. Now you just have to make sure to remove those handlers when the component is destroyed!

Hi everyone,

ember-concurrency has progressed quite a bit in the last few weeks and at this point I can recommend that you start using it in your apps. It really strikes at the root of so many problems (some you didn’t even know you had) with managing async in Ember with only promises/callbacks at your disposal. If you look at the docs today and it’s still unclear how it can help your app, please let me know, and I’ll do what I can to make things even more clear.

If I’ve done my job with ember-concurrency you’ll never have to check this.isDestroyed, or implement a hook to cancel a timer, or implement your own isLoading/isRunning/isProcessing flags, or write boilerplate code to prevent a task from running concurrently, ever again.

EDIT: if you’re wondering where to start, check out the website: http://ember-concurrency.com/

1 Like

@machty — I should have followed up on this thread myself. I cannot express how excited I am about ember-concurrency now since the latest version landed. (Currently in the midst of refactoring so so many things to using it).

As a consumer of open source and a somewhat-infrequent-contributor-and-not-proud-about-it, thank you. (Not sure I ever expressed a thank you when you delivered the async router three years ago too ;-)).

2 Likes

@machty On this post: So have you tried ember-concurrency yet? you stated:

Never write an if(this.isDestroyed) check ever again 1 → instead, use a Task, which gets automatically canceled when the object it lives on is destroyed

I’m not sure that’s completely fair. If I understand right, tasks are essentially decorators? You can’t use them anonymously, right? If that’s true, you’d have to create a class function for every promise handler (resolve/reject/finally) right?

Maybe there is a way to make it work that I’m not seeing. Given the first code snippet in my original question, how would you modify that to use a task so I didn’t have to check isDestroyed?

I’m not sure I follow what you’re saying about having to define a class function, but my point was that if you yield aPromise, if that promise fulfills, then you continue where you left off (just as yield timeout(123) continues where you left off once the timer elapses), if the promise rejects, it’s treated like an exception thrown from that point in the generator function (which fwiw is exactly how it works when you await aPromise in the proposed async-await syntax), but if, while that promise hasn’t yet resolved, the task is canceled because the object it lives on was destroyed (e.g. a Component is unrendered) then you’re guaranteed that it will NOT resume the generator function and start doing a bunch of this.set('prop', 'boom')s on a now-destroyed object.

So I can’t think of a situation, if you’re using ember-concurrency tasks and the yield keyword (rather than a chain of promises), where you’d wind up running code outside the lifetime of an object. Let me know if I’m misunderstanding you though.

@machty What I mean is that if you take this exact code snippet:

// app/components/x-routable/post.js

export default Ember.Component.extend({
  /* ... */
  actions: {
    saveDraft() {
      this.set('isSavingDraft', true);
      this.get('post').save().finally(() => {
        this.set('isSavingDraft', false);
      });
    }
  }
});

The real problem is that once save has completed the component might be destroyed. (That part is obvious). But I don’t think I can use task on an action, right? Eg:

export default Ember.Component.extend({
  /* ... */
  actions: {
    saveDraft: task(function() {
      this.set('isSavingDraft', true);
      yield this.get('post').save();
      this.set('isSavingDraft', false);
    })
  }
});

It looks like task relies on some of the Ember class machinery to do it’s job. If that’s true, then I’d have to create a separate task function on the class to handle each action, right?

Ah, I see, right: tasks can’t (at present) be specified in the actions hash due to how Ember’s class machinery works. I could try and find a way to facilitate this, but then that would also just make it more indirect for a button to bind to the state of the task; rather than just writing {{if saveDraft.isRunning ...}} you’d have to do {{if actions.saveDraft.isRunning}}. So instead, tasks live directly on the class, and tasks can be invoked by passing nameOfTask.perform into the action helper, or into components as actions (this page shows all the ways to perform a task).

Yeap. So I could to this:

export default Ember.Component.extend({
  /* ... */
  saveDraft: task(function() {
    this.set('isSavingDraft', true);
    yield this.get('post').save();
    this.set('isSavingDraft', false);
  }),

  actions: {
    saveDraft: function() {
      this.saveDraft();
    }
  }
});

But it would create more boilerplate. ¯\_(ツ)_/¯. No biggie, I just wanted to make sure that I understood the capabilities.

BTW setting isSavingDraft for is just a trivial example. Typically we’re manipulating and storing result data from the promise.

@workmanw: I’m saying you don’t put anything in the actions hash at all.

export default Ember.Component.extend({
  /* ... */
  saveDraft: task(function * () {
    this.set('isSavingDraft', true);
    yield this.get('post').save();
    this.set('isSavingDraft', false);
  }),
});

and in your template:

<button {{action saveDraft.perform}}>Click me</button>

@workmanw Furthermore: for what it’s worth, having to set and then unset isSavingDraft before and after the async operation is part of the boilerplate ember-concurrency is trying to get rid of. You could simplify the above into:

export default Ember.Component.extend({
  isSavingDraft: computed.alias('saveDraft.isRunning'),
  saveDraft: task(function * () {
    yield this.get('post').save();
  }),
});

or just

export default Ember.Component.extend({
  saveDraft: task(function * () {
    yield this.get('post').save();
  }),
});

and then just put {{#if saveDraft.isRunning}} in your template wherever you need it. Another example of this is in the Loading UI example in the docs.

Huh. How about that. I completely forgot that {{action}} would support functions now. So yea, I guess that does make sense.

Yea, really loading state is just a basic example. More often than not we’re fetching “more details” for a component, then storing that data onto the component so it’s available for the template.

Anyways, yea I get the approach and everything. Thanks for the good back and forth!

ember-concurrency is wonderful, but I still don’t think it quite resolves the issue of isDestroyed. It pretty much depends on the timing. A component can be destroyed right after a successful resolve of a promise. We could extend @spencer_price 's idea by overriding the set function with the following mixin (say SafeSetMixin)

export Ember.Mixin.create({
  set(key, val) {
    if (!this.isDestroyed) {
      this._super(key, val);
    }
})

Then your component can just extend from that, and the rest of your component code doesn’t need any modification with regard to its set calls.

export Ember.Component.extend(SafeSetMixin, {
...
})

Under what circumstances does Ember-concurrency not handle this correctly? I was under the impression that it handles the case where a component is destroyed mid task and does not process to the next step. So far, I’ve not run into this — but curious where you have!

Nope. JavaScript fakes asynchronicity with an event loop. It is not actually multithreaded. You can’t have such a “task switch” in any current implementation of JavaScript without using something like web workers, and even in that case memory isn’t shared.

If it could, even your proposed code wouldn’t be safe, and you would need real threading primitives like mutexes to handle it.

In my case, my component has this logic:

updateCaseFileListTask: task(function*() {
  try {
    let caseFiles = yield this.caseService.getCaseFiles();   // promise to resolve to caseFiles
    this.set('caseFiles, caseFiles);       // <== Exception thrown here
    ...
  } catch (err) {
   ...
  }
}).drop(),

Typically, it doesn’t happen often. But if I keep quickly navigate back and forth to/from the page that contains this component (doing it consecutively many times), it does occur sometimes (perhaps 1 out of 15 chances).

@tpham Simply place your promise in a child task.

@Gaurav0 I believe you’re correct. Yet it still leaves me scratching my head. Just to clarify a bit more, the task is performed upon my component is inserted into the DOM:

didInsertElement() {
   Ember.run.next(this, function() {
     this.get('updateCaseFileListTask').perform();
  });

}

@tpham The issue is simply that a task is canceled when the owner object is destroyed. The task will still run to completion, it will simply call its catch rather than its then if it is canceled. Your promise this.caseService.getCaseFiles() has no such guarantee.