Switch to RxJS #12384
Switch to RxJS #12384
Conversation
|
🦋 Changeset detectedLatest commit: 52e4027 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for apollo-client-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
commit: |
| // XXX this looks like a bug in zen-observable but we should figure | ||
| // out a solution for it | ||
| it.skip("handles an unsubscribe action that happens before data returns", async () => { | ||
| it("handles an unsubscribe action that happens before data returns", async () => { |
There was a problem hiding this comment.
Now that we are using RxJS, this test passes 🎉
| expect(onRequestSubscribe).toHaveBeenCalledTimes(2); | ||
| expect(onRequestUnsubscribe).toHaveBeenCalledTimes(2); | ||
| }); | ||
|
|
||
| it("supports interoperability with other Observable implementations like RxJS", async () => { |
There was a problem hiding this comment.
Now that we are using RxJS, this test doesn't make much sense. I'd rather not install another library either just to replace this test so I nuked it.
| @@ -1491,6 +1565,108 @@ describe("ApolloClient", () => { | |||
| }); | |||
|
|
|||
| it("supports cache-only fetchPolicy fetching only cached data", async () => { | |||
There was a problem hiding this comment.
This is something else I fixed in this PR since it was easier to fix now than try and work around it. cache-only queries that don't use returnPartialData: true won't emit a partial data value anymore. I added some additional tests that verify partial data with returnPartialData set/not set.
| .then(() => { | ||
| mutationComplete = true; | ||
| }); | ||
| await client.mutate({ mutation, refetchQueries: ["getAuthors"] }); |
There was a problem hiding this comment.
The description of the test includes the text "when awaitRefetchQueries is undefined", yet awaitRefetchQueries: false was passed to this test. I aligned the test with the description here to ensure this works as expected.
I also did a bit of refactoring here to try and better show off that the refetch wasn't waited on by using await on the mutation rather than trying to track that with a mutable boolean variable.
| await client.mutate({ | ||
| mutation, | ||
| refetchQueries: ["getAuthors"], | ||
| awaitRefetchQueries: false, |
There was a problem hiding this comment.
Similarly, the test description says "when awaitRefetchQueries is false", so I updated it here. Seems this test and the other were swapped 😆
| expect(observable.getCurrentResult()).toEqualApolloQueryResult({ | ||
| data: secondReqData, | ||
| loading: false, | ||
| networkStatus: NetworkStatus.ready, | ||
| partial: false, | ||
| }); | ||
| expect(mutationComplete).toBe(false); |
There was a problem hiding this comment.
This was an example to me that was especially egregious. All this proved was that the promise from mutate hadn't resolved by the time this assertion ran, but this didn't quite prove to me that the functionality worked. By using await then asserting, its more obvious whether it actually awaited the refetch before resolving.
|
|
||
| expect(context.foo).toBe("bar"); | ||
| } | ||
| ); | ||
|
|
||
| it("`defaultContext` will be applied to the context of a subscription", async () => { |
There was a problem hiding this comment.
I went ahead and split this out to a separate test since you need to call .subscribe again to actually kick off the subscription since its now evaluated lazily unlike the old Concast approach.
| { | ||
| method: "query", | ||
| option: "query", | ||
| document: gql` |
There was a problem hiding this comment.
I added this document here so that the operation type matched the method we were calling. Felt a bit weird to call client.mutate(...) with a query. While it doesn't matter a ton for this test, I'd prefer to be as correct as possible if we can help it.
| @@ -76,6 +76,7 @@ | |||
| "graphql-ws": "^5.5.5 || ^6.0.3", | |||
| "react": "^17.0.0 || ^18.0.0 || >=19.0.0-rc", | |||
| "react-dom": "^17.0.0 || ^18.0.0 || >=19.0.0-rc", | |||
| "rxjs": "^7.8.0", | |||
There was a problem hiding this comment.
TODO: Determine the proper range of allowed versions.
| @@ -3475,6 +3480,7 @@ describe("useQuery Hook", () => { | |||
| expect(result).toEqualQueryResult({ | |||
| data: undefined, | |||
| error: new ApolloError({ graphQLErrors: [{ message: "error" }] }), | |||
| errors: [{ message: "error" }], | |||
There was a problem hiding this comment.
These are temporary since we are deprecating the errors property. At least this is more consistent 🤣
| setResult(result, resultData, observable, client, handleStoreChange); | ||
| }; | ||
|
|
||
| const onError = (error: Error) => { |
There was a problem hiding this comment.
ObservableQuery no longer emits an error event so we don't need this error handler anymore 🎉
src/react/hooks/useQuery.ts
Outdated
| // let subscription = observable.subscribe(onNext, onError); | ||
| const subscription = { current: observable.subscribe(onNext, onError) }; | ||
| // let subscription = observable.subscribe(onNext); | ||
| const subscription = { |
There was a problem hiding this comment.
I think we can remove this mutable object now that the error handler is gone and doesn't reassign this value. Will update.
jerelmiller
force-pushed
the
jerel/rxjs
branch
from
aa8e6cc to
82b6e78
Compare
|
@PowerKiKi I'm tagging you on this PR in case you're interested. We'd like to cut a first alpha of 4.0 some time after we get this branch merged into the 4.0 release branch and would love if you'd be willing to help find any edge cases with this switch over. Feel free to take a look through this PR if you'd like to provide any early feedback. Thanks for all your prior work on this issue! |
|
Great work on these improvements—switching to RxJS and updating Observable behaviors is a big step forward On a related note, with Chrome 135 now enabling the Observable API by default, these changes become even more relevant. Native support for Observables in Chrome can help streamline development and might reduce the need for extra polyfills or workarounds in supported environments. It might be worth discussing whether we can leverage this native implementation further in our client code. |
| inFlightLinkObservables.remove(printedServerQuery, varJson); | ||
| } | ||
| }), | ||
| shareReplay({ bufferSize: 1, refCount: true }) |
There was a problem hiding this comment.
| shareReplay({ bufferSize: 1, refCount: true }) | |
| shareReplay({ bufferSize: 1, refCount: true }) |
Bump the bufferSize
Closes #5295
Closes #12183
Switch to RxJS as the Observable implementation used throughout the client. Along with this, several behaviors have changed:
Errors emitted from link observables no longer terminate
ObservableQueryRather than emitting an
errorevent and terminating theObservableQuery, errors are now emitted on theerrorproperty as anextevent. This makes the behavior more predictable when callingrefetch,fetchMore, etc after an error occurred. Previously you'd have to do some work arounds to get notified of new values after executing new requests.Unsubscribing from all subscriptions on
ObservableQueryno longer terminates pending requestsThis is behavior that was implemented in
useLazyQueryand brought to the rest of the client. When starting requests either by subscribing toObservableQuerythe first time, callingrefetch, etc. will no longer terminate if unsubscribing fromObservableQuery.ObservableQuerywill still get torn down and no events emitted, but the request will complete. This allows you to receive data from the promise returned from these methods rather than having to catch and detect if the error was a result of terminating the outgoing request.For the old behavior, we encourage the use of an
AbortControllerinstead.cache-onlyqueries will now emit partial data only whenreturnPartialDatais setPreviously
cache-onlyqueries would emit a partial result regardless of whetherreturnPartialDatawastrueorfalse. To make the behavior consistent across all fetch policies, this has been updated.Now that we are using RxJS, the following utilities have been removed in favor of using RxJS operators instead:
asyncMapfromPromisetoPromisefromErroriterateObserversSafelyConcast