How to do you check for `isDestroyed`?

Design
21

@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?

22

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).

23

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.

24

@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>
25

@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.

26

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!

27

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, {
...
})
28

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!

29

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.

30

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).

31

@tpham Simply place your promise in a child task.

32

@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();
  });

}

33

@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.

34

@Gaurav0 Wouldn’t the catch block in updateCaseFileListTask be executed if the component is destroyed (that’s what I expect)? The getCaseFiles is simple really - just have Ember data return the data:

getCaseFiles() {
   return this.store.findAll('casefile');
}

I’ll try child task nonetheless. Thanks.

35

I think if you’ve got a try/catch and the task is canceled, the current pattern is to check wether the error was a task cancelation. Something like:

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

FYI: It is in fact TaskCancelation and not TaskCancellation (see this troll-ish issue I opened).

1 Like
36

Very true. Thank you for the reminder.