Add Ember.run.callback()

[davewasmer]

Rendered

[stefanpenner]

Run is part of metal, and register waiter is part of test, i am unclear if this should be part of run namespace.

[davewasmer]

That's fair. My thought was that the behavior here matches similar behavior with the other Ember.run.* methods (i.e. want an Ember aware setTimeout? run.later. Want an Ember aware callback? run.callback). It mixes concerns under the hood, but from my perspective when trying to solve this problem, Ember.run was my gut reaction first place to check.

Certainly open to suggestions of a better spot for it to live.

[stefanpenner]

can side-by side examples be provided comparing this to registerWaiter?

[davewasmer]

@stefanpenner added example code for both current approaches in the Alternatives section (manual waiters and direct state manipulation).

I took a stab at the example code, but if either one seems uncharitable to the current alternatives, feedback is welcome.

[courajs]

The return value of of Ember.run.callback()

[davewasmer]

Good catch 👍

[stefanpenner]

The manual registration example is somewhat strange. Wouldn't the waiter be in the app code, so that all tests interacting with that entity would work the same?

Ember.run.callback itself has a fairly vague name, I could users using it in place of run.bind and being confused by the outcome.any ideas?

Another thing, which types of callbacks should this be passed to, all/any/some. How can we make users fall into a pit of success.

[davewasmer]

The manual registration example is somewhat strange. Wouldn't the waiter be in the app code, so that all tests interacting with that entity would work the same?

Fair point. I'll add an example demonstrating that.

Ember.run.callback itself has a fairly vague name, I could users using it in place of run.bind and being confused by the outcome.any ideas?

Totally agree, I don't really like the name either, I just couldn't think of a better one 😉

It does bear some similarity to run.bind(), since you return a function rather than immediately invoking something (i.e. like schedule()), and the return function should be used as a callback function itself, so that's what lead me to callback(), but suggestions are certainly welcome.

Another thing, which types of callbacks should this be passed to, all/any/some. How can we make users fall into a pit of success.

It should be used for any async callbacks that aren't already tracked by Ember.

I can see a few places where users might run awry:

1. Using callback() for already-tracked asynchrony. I.e. new RSVP.Promise(Ember.run.callback(...)). This could be straightforward to guard against; Promise, ajax, other run methods, etc could all check to see if the function they received was produced by run.callback(), and warn if so.
2. Using callback() for synchronous callbacks. I don't see any bugs this would produce, but it's not strictly necessary. Perhaps we could try to detect if the callback is invoke synchronously and warn the user?
3. Assuming that it behaves more like schedule() than bind(), i.e. assuming immediate invocation. This one is trickier to guard against (not sure how we could detect that the function wasn't invoked). The ugliest form of this is if it occurs during tests, which would simply cause them to hang with no explanation. Perhaps some kind of debugging tool to inspect what waiters the app is currently waiting on? Although that's probably outside the scope of this RFC.

@stefanpenner do these points belong in the RFC somewhere (Unresolved Questions)? Or should we let the discussion play out first?

[stefanpenner]

Using callback() for already-tracked asynchrony. I.e. new RSVP.Promise(Ember.run.callback(...)). This could be straightforward to guard against; Promise, ajax, other run methods, etc could all check to see if the function they received was produced by run.callback(), and warn if so.

This isn't an async method, and we don't really have control checking this from the callee side. Literally any-place that we would want to use this, is more or less by definition out of our control. $(..).click setTimeout etc.etc.

Perhaps some kind of debugging tool to inspect what waiters the app is currently waiting on? Although that's probably outside the scope of this RFC.

Maybe, but this would be valuable as well (maybe a different RFC) aka, named waiters that track their callstack that invoked them during testing for easy inspection.

Introducing a high-level API does usually mean we need to investigate improved debug-ability.

Assuming that it behaves more like schedule() than bind(), i.e. assuming immediate invocation. This one is trickier to guard against (not sure how we could detect that the function wasn't invoked).

This isn't the hazard I'm concerned with, let me demonstrate, given:

this.$().click(Ember.run.bind(this, this.method));

If someone was like, oh a callback, I should use Ember.run.callback in some cases this will be desired and in others it won't be. It may even cause unexpected, or unexplained test hangs.

won't be:

didInsertElement() {
  this.$().click(Ember.run.callback(this, this.method));
  // testing would just hang when the component was rendered, regardless of intent.
}

will be:

didInsertElement() {
  this.$().click(_ => {
    $(this).one('didLoad', Ember.run.callback(this, this.method)); // likely works as expected
  });
}

---

I do think we need to improve this, (i literally spent 2 days last week tracking down related issues in test suites). But maybe first several common scenarios that don't pause tests but should should be enumerated. That way we can look at each case, and decide what the appropriate solution is (I would venture to guess, we could do more automatically then we do today).

Another thing to consider, is testWaiters can be aborted but callback in this RFC cannot. Their is no way (currently) to say, oops NVM no longer block tests. Here is a quick example (inspired by one of the fixes I made last week);

let complete = false;
Ember.Test.registerWaiter( _ => complete);

this.one('didLoad', _=> complete = true); // assume cleanup of the error listener
this.one('error', _=> complete = true);     // assume cleanup of the error listener

[davewasmer]

@stefanpenner:

This isn't an async method, and we don't really have control checking this from the callee side. Literally any-place that we would want to use this, is more or less by definition out of our control. $(..).click setTimeout etc.etc.

My point was the opposite - that we might want to warn if someplace inside our control receives a callback() wrapped function, since like you pointed out, by definition that's likely a mistake. Or perhaps I'm misunderstanding your comments ...

If someone was like, oh a callback, I should use Ember.run.callback in some cases this will be desired and in others it won't be. It may even cause unexpected, or unexplained test hangs.

Ah, okay. Your examples make sense. Yea, that could be tricky, and I don't see a good way of preventing that apart from choosing a better name and good docs, since there isn't a great way to tell those use cases apart in an automated fashion (AFAICT).

But maybe first several common scenarios that don't pause tests but should should be enumerated.

Sure. Should I add these additional examples to the RFC doc? Suggestions of scenarios are welcome.

Another thing to consider, is testWaiters can be aborted but callback in this RFC cannot.

The callback can be canceled. From the RFC:

If the callback is no longer needed, the user could cancel the waiter by passing the callback to Ember.run.cancel() (matching the other async Ember.run.* methods).

This is implemented in the addon spike. Are you looking for some other kind of "abort" behavior?

[stefanpenner]

Ah. I must have missed the cancellation aspect.

Yes I think enumerating the use-cases explicitly would be good. It will help this discussion, and maybe spark complementary solutions.

[lolmaus]

You have two Current solutions in the RFC which illustrate quite vividly the pain this RFC is aiming to resolve. 👍

But can you please add a code sample to the RFC demonstrating what those Current solutions could be replaced with?

[lolmaus]

Oops, didn't notice this RFC is old!

Is it still relevant?

[wagenet]

I'm going to assume this is no longer relevant. If I'm incorrect, let me know and I'll see what I can do to get it moving forwards again.