Make subscription available inside callback

[dmurvihill]

This code fails when I run it in test:

await new Promise(resolve => {
  const subscription = db.myData().observe(s => {
    subscription.unsubscribe();
    t.deepEqual(s, expectedData);
    resolve();
  });
});

ReferenceError {
    message: 'subscription is not defined',
  }

It fails because db is a mock database where myData() calls the callback synchronously and immediately, meaning the callback executes before the assignment operator. It would be really nice if this code would work regardless of whether the myData() implementation is synchronous or asynchronous.

Maybe we could make the subscription object available inside the callback?

This is what I am doing for now:

function delay(ms: number): Promise<void> {
  return new Promise(r => setTimeout(r, ms));
}

await new Promise(resolve => {
  const subscription = db.loadData().observe(s => {
    delay(1).then(() => {
      subscription.unsubscribe();
      t.deepEqual(s, expectedData);
      resolve();
    })
  });
});

[hallettj]

It looks like you want to wait for the first event (either a value, an error, or the end of the observable), and then make an assertion. I would do that like this:

// create an observable that ends after the first value or error
const firstEvent = db.myData().take(1).takeErrors(1)

// convert the observable to a promise that resolves when the observable ends
await firstEvent.toPromise()

// make assertion here

[mAAdhaTTah]

Setting aside what you could do about Kefir in this case, I'd start off by suggesting that your mock is buggy. If the implementation you're mocking is async, your mock should be async as well. The fact that it isn't means the behavior that you're modeling with the mock has different semantics, and the tests that you write aren't going to match the behavior in the real app, causing subtle bugs your test suite won't catch.

Making observe always async would be a major breaking change in the semantics of Kefir's observables, and one that I'm not sure is broadly beneficial. The behavior of Property would end up very different, because code that is depending on observe's synchronous behavior if there's a current event in the Property would now break.

Broadly though, I'd argue you should seek to write your code such that it doesn't actually matter whether it's sync or async, because the Observable abstracts that away from you such that you mostly don't need to know. Obviously, this isn't feasible in every case, but thinking at a higher level about what you're trying to accomplish often leads to solution where it is, and this is one of those cases.

@hallettj's solution is spot on here, since what you're actually trying to do it is get the first value and assert against it, so take(1) is definitely the way to go. Whether you decide to take(1).observe() or take(1).toPromise() and use the value that way depends on larger contexts, although I tend to prefer to stay in "Observable land" as much as possible so would prefer the former.

I'm going to close this issue, as there's nothing actionable here (we can't change `observe) to be always-async) but I'm happy to help if you have follow-up questions, so feel free to reply to this issue.

[dmurvihill]

I didn't implement the mock and don't control it, and to turn your argument around I'd argue you should seek to implement observe such that it doesn't actually matter whether the subscribe callback is sync or async. After all, the goal is to abstract it away such that the caller mostly doesn't need to know and that is not accomplished here.

The answer is not to make observe always asynchronous -- nobody asked for that -- but rather to pass the subscription object into the onValue and onError callbacks as a parameter, as I suggested above. The subscription always exists when the callback is executing (I sincerely hope) so it should be a non-breaking change to pass it as a second parameter.

I'll follow up with a pull request. In the meantime, I will take @hallettj's advice and use take.

[dmurvihill]

298

[mAAdhaTTah]

I wouldn't want to encourage people to use the callback to onValue in the subscribe callback to determine whether or not to cancel a subscription. There are a number of operators which you can use to trigger an completion of the observable. In your example, the answer was to use take(1). There are others (takeWhile comes to mind as relevant to the discussion here), but broadly, within the onValue callback isn't where that decision should be made, so to speak.

So I'd be opposed to the proposed change in #298 but I'd be interested in hearing from other maintainers on this.

[hallettj]

I'm also opposed to making the subscription object available in the callback. I think it's an unnecessary API complication.

[mAAdhaTTah]

@kefirjs/core Anyone in support of this change? If not, I'm going to close this out.

[dmurvihill]

It's been 7 days now and no positive interest so I'm going to go ahead and close this. Cheers and thanks for helping out w/ alternatives.