I think the breaking change would also be in this scenario:

observable.next(10)
observable.subscribe()

I think there's a difference in ZenObservable and RxJS on how they emit and listen data (sync/async).

Oh and thanks for continuing. My goal was to add RxJS to Apollo Client and later on replace ApolloLink with RxJS to use retry/debounce etc from rxjs/operators (instead of duplicating the same logic).

I think there's a difference in ZenObservable and RxJS on how they emit and listen data (sync/async).

I think it is one of the issue I am struggling with, Concast now emits "too fast" and subscribers cannot react as they used to, because they don't see early emits. That the reason why I always goes through a promise now:
https://github.com/apollographql/apollo-client/pull/9331/files#diff-c1fb05328a28ee974640fa4441740670ccd20ea2fbddae1230abec0c61c8c633L77-R85

But it does not solve everything.

Re-reading my original message, I might not have been clear enough. I am now waiting for a confirmation from core team, probably @benjamn or @hwillson, that this PR still have a chance to get merged eventually, and how to handle Concast (whether we can break its API by migrating entirely to RxJS).

@PowerKiKi as a suggestion, now that you're using RxJS, it's very easy to make any API you previously had accepting only Observable<T> also accept Promise<T> | AsyncIterable<T> | Iterable<T> | ReadableStream<T>. All RxJS APIs that accept Observable<T> will accept the others. You'd just need to type your stuff as ObservableInput<T>.

For example, ApolloLink.from([() => of('hi')]) could just as easily be ApolloLink.from([async () => 'hi']), or ApolloLink.from([async function* () { yield 'hi'; }), or even ApolloLink.from([() => [of]]) with almost no code changes on your part.

@PowerKiKi if you need help with scheduling, let me know. RxJS has APIs to get the forced scheduling that zen-observable seems to have added on top of the original proposal. Arguably it would be better not to have to depend on that, but either is doable.

@benlesh thank you for the advices.

Usage of ObservableInput<T> sound like a great addition, but I'd rather wait until we pass existing tests, before adding new things that can break 🤓

It seems like asapScheduler would help reproduce zen-observable scheduling. But, like you said, I am not quite sure it would reasonable to force all observables to be created with that scheduler, or to "reschedule" all observable via observeOn 🤔

If core team can confirm their interest in this PR, then I can try harder to make it work without a specific scheduler.

FYI: You should make sure this requires 7.5.4 for even better performance. Just testing this out, and [email protected] is about 10x faster in the best case than zen-observable, and about 0.4x faster in the worst-case.

This is the average time to emit 1,000,000 values in milliseconds:

By version 8.0 we're also going to try to have a separate package for people that just want to use Observable on its own (even though the main package is completely tree-shakable, it seems to present a mental boundary for some folks that rxjs is "batteries-included")

From what I understand this PR would remove such issues by letting us replace the Observable implementation that Apollo uses. Which means nextLink.request(op) would actually return a rxjs Observable .

Is my understanding correct?

The reason I'm asking is that we recently ran into a very hard to track down issue when creating a custom link:

new ApolloLink((op, nextLink) => {
   const dummyRxObsThatOnlyReturnsOneItem$ = of(true);

   return dummyRxObsThatOnlyReturnsOneItem$.pipe(
      // while nextLink.request(op) was detected as compatible return type
      // it caused intermittent runtime errors along the lines of "Object was not a stream, array, buffer"
      // by explicitly wrapping the "apollo/link" observable in a rxjs observable the error disappeared, 
      // no other changes were made
      switchMap(() => fromLinkObservable(nextLink.request(op)))
   ),   
})

export function fromLinkObservable<T>(obs: ApolloObservable<T>): Observable<T> {
  // the below Observable is imported from `rxjs`
  return new Observable<T>((observer) => {
    const sub = obs.subscribe({
      next: (result) => {
        observer.next(result);
      },
      error: (error) => {
        observer.error(error);
      },
      complete: () => {
        observer.complete();
      },
    });

    return () => {
      if (!sub.closed) {
        return;
      }
      sub.unsubscribe();
    };
  });
}

Yes this PR would entirely replace zen-observable-ts by rxjs, which nowadays is clearly the better alternative of the two.

The bug you are experiencing is likely to be related to the difference of scheduling mechanism mentionned in #9331 (comment), #9331 (comment) and #9331 (comment). So in a way yes this PR should fix the unexpected behavior you noticed.

Hey all 👋

Thanks for the patience with all of this! I've been doing a bit of housekeeping in this repo and came across this PR. I'm super interested to see if we can revive this and figure out how best to slot it in an an upcoming release. Sounds like @hwillson and @benjamn are in support of the switch to RxJS. Could we start with rebasing against the latest changes in main?

Also since observables are exposed by Apollo, and observable signatures change, then this whole PR would be a breaking change

I'm not super familiar with RxJS unfortunately so my question is (apologies if this is naive): Are there any APIs in zen-observable that flat out won't work with RxJS observables? Would RxJS observables be a drop-in replacement?

We currently extend zen-observable's Observable with a class called ObservableQuery which is what we expose with our React hooks/client functions. I'd be curious if we can have a bit more of a comprehensive look at the differences to understand if we are in fact introducing a breaking change or whether the ObservableQuery abstraction would hide this fact.

We are planning a 4.0 at some point, likely next year, so if there are in fact breaking changes, perhaps we look to slot it in for the 4.0 release.

Other than some tests failing, what is needed to see this through to completion?

@PowerKiKi 1 more question! With this PR up, do we need #5749 open anymore? I'd like to close old/stale PRs if possible and would rather work off this iteration if the other one is stale. Thanks!

rebasing against the latest changes in main?

I hopefully will have a bit of time tomorrow to make test pass. After that, we can rebase.

flat out won't work with RxJS observables?

Since I am not super familiar with the details of zen-observable, I am not entirely sure. It might be mostly compatible API-wise. However there are subtle differences that could be considered a breaking change. What I am refering to is the timing when things emits, see #9331 (comment) and following. That's the bit I am struggling with in tests...

what is needed to see this through to completion?

Pass tests and rebase :D

do we need #5749 open anymore?

IMHO it should be closed, because this is a direct continuation of it (with the approval of OP).

hopefully will have a bit of time tomorrow to make test pass. After that, we can rebase.

Ya no problem at all! No rush on this, I just wanted to start the conversation again about getting this change into the client and make sure this PR didn't fall by the wayside.

it might be mostly compatible API-wise

This might be something we'll need to explore a bit more in-depth to figure out how we'd release this change and whether this would make sense as a minor or major release. I have no idea the kinds of creative things Apollo client users are doing with the returned observable and whether any change would actually break something, but probably best to err on the side of caution (though I suspect interacting with it directly is less common).

Hey all 👋

We've added this as an item to our 4.0 wishlist and something we are seriously considering. To avoid the effort needed to keep this up-to-date with main for the forseeable future, I'm going to go ahead and close this PR. We'll make sure to use this as a reference, or resurrect it when the time comes to put more development effort into 4.0.

Thanks a bunch for all the exploratory work you've done here to make the switch possible!

That may be for the best. I obviously could not invest as much time as was needed to complete this PR. Sorry about that.

But also I'm now quite convinced that this work will definitely introduce breaking changes in the timing of things, because rxjs is "faster" than zen, and I think it's is best to adopt rxjs default scheduling, rather than force a specific scheduling for each observable created. That means that Apollo users will have to be a bit more careful when they subscribe as they might immediately and synchronously receive a value.

Also, leveraging rxjs pipe system to entirely replace the concept of Apollo Links, as suggested by Ben, would seems like a great thing to do. Much less code to maintain for Apollo, and more "standard" for Apollo users that are already familiar with rxjs. But that would be a huge breaking change that should most likely be done by Apollo core team directly.

So all of this should definitely go into a major version.

@PowerKiKi totally fine! Now that I'm more caught up to speed with this project, I'm realizing keeping this PR up-to-date could be a maintenance nightmare as more and more is changed. That's a burden that a community member like yourself shouldn't need to deal with.

Appreciate all your work and learnings here though. This will be hugely valuable when we pick up development on v4 as we should be able to leverage a lot of this. Thanks!

@jerelmiller I just pushed PowerKiKi@a8ce585, which somehow doesn't appear here. It is still very incomplete, but I thought it might be of interest anyway. In particular I try to replace the class Concast by a new function concast(). I think the idea is the right one, though the work is unfinished, and it definitely is a breaking change, as I've said before.