Switch to RxJS#12384
Conversation
|
🦋 Changeset detectedLatest commit: b5577e3 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: |
jerelmiller
force-pushed
the
jerel/rxjs
branch
from
de61c1d to
8ce76f2
Compare
size-limit report 📦
|
| await expect( | ||
| obs.setOptions({ query, fetchPolicy: "standby" }) | ||
| // TODO: Update this behavior | ||
| ).rejects.toThrow(new EmptyError()); |
There was a problem hiding this comment.
This will be done in a separate PR. I think it makes sense that if no value is emitted before complete is called, an error is thrown. We'll create our own network error for this instead though that better describes what the issues is. EmptyError is deprecated in RxJS anyways and is considered an internal implementation detail.
| const error = await stream.takeError(); | ||
|
|
||
| expect(error.message).toBe("Uh oh!"); | ||
| await expect(stream).toEmitApolloQueryResult({ |
There was a problem hiding this comment.
This is one of the big changes in this PR. Rather than emitting an error event, a next event is emitted instead with an error property (similar to errorPolicy: 'all'). The great thing about this change is that it no longer terminates the observable which means we can continue to receive updates after errors and removes some of the confusion on what to expect when calling APIs such as refetch, etc. This also allows us to remove observers tracking in ObservableQuery since we can now just emit new values on the underlying observable rather than trying to get around the terminating behavior 🎉
|
|
||
| expect(aResults).toEqual([]); | ||
| expect(bResults).toEqual([]); | ||
| expect(aResults).toEqual([{ a: 123 }]); |
There was a problem hiding this comment.
Observable values are now emitted synchronously so these tests now have data to check against.
| request: { query: mutation }, | ||
| result: mutationResult, | ||
| expect.assertions(7); | ||
| const link = new MockLink([ |
There was a problem hiding this comment.
I inlined the setup function implementation because I needed to remove the extra .subscribe call that is in the setup process for this test to work properly.
| subs.push( | ||
| extraObs.subscribe({ | ||
| next(result) { | ||
| expect(result).toEqual({ a: "A", b: "B" }); |
There was a problem hiding this comment.
This assertion was never run apparently because the assertion was just plain wrong. Fun to get this finally working as expected.
| // ignoring it here. | ||
| this.reobserve().catch(() => {}); | ||
| } | ||
| this.initialResult = { |
There was a problem hiding this comment.
This is likely temporary and will be replaced once we do the work to make notifyOnNetworkStatusChange behave more predictably. Once that work is done, we'll have no need to compare using === for the initial result. Doing so in the filter function below is mostly for backwards compatibility until the other PR is complete.
| filter( | ||
| (result) => | ||
| // TODO: Remove this behavior when unifying loading state for notifyOnNetworkStatusChange | ||
| (this.options.fetchPolicy === "no-cache" && | ||
| this.options.notifyOnNetworkStatusChange) || | ||
| // TODO: Remove this behavior when unifying loading state for notifyOnNetworkStatusChange | ||
| (this.options.fetchPolicy === "network-only" && | ||
| this.queryInfo.getDiff().complete) || | ||
| result !== this.initialResult | ||
| ) |
There was a problem hiding this comment.
This is still sorta incorrect, but I tried to note where I had to add extra assertions in other tests for emitted loading states when first subscribing. Eventually this will be replaced with a simple check to see if notifyOnNetworkStatusChange is set and the result is loading.
| return this.observable.subscribe(observer); | ||
| } | ||
|
|
||
| pipe(): Observable<ApolloQueryResult<MaybeMasked<TData>>>; |
There was a problem hiding this comment.
Recommendations welcome on simplifying these overloads. I copied this from RxJS in the mean time.
There was a problem hiding this comment.
Delete all of that (subscribe and pipe) and add
public readonly subscribe: Observable<
ApolloQueryResult<MaybeMasked<TData>>
>["subscribe"];
public readonly pipe: Observable<
ApolloQueryResult<MaybeMasked<TData>>
>["pipe"];somewhere as a declaration and
this.subscribe = this.observable.subscribe.bind(this.observable);
this.pipe = this.observable.pipe.bind(this.observable);in the constructor? :)
Oh, and a readonly on the observable property declaration to ensure we never reassign it.
There was a problem hiding this comment.
I had to tweak the TS type a little bit: 6ea08e0
Turns out that its very difficult to infer T with the currently types of Observable from RxJS. The following results in unknown:
type GetObsType<T> = T extends Subscribable<infer S> ? S : never;
type T = GetObsType<Observable<ApolloQueryResult<{foo: string }>>;
// ^? unknownThe .subscribe interface that accepts 3 functions is deprecated anyways, so by limiting the type to just the 1 callback, or partial subscriber seems to work.
| return (this.observable as any).pipe(...args); | ||
| } | ||
|
|
||
| // TODO: Consider deprecating this method. If not, use firstValueFrom helper |
There was a problem hiding this comment.
I welcome feedback for all methods I've marked with potential deprecation.
This one is particular doesn't seem useful. I found this comment which makes me think there was never much purpose for this method.
Once I do the work to make the observable terminate when this is torn down, this function is going to be problematic anyways since it creates a subscription then unsubscribes, which means it will end up terminating this observable by using this function. To be honest, I don't really want to find a way around that issue.
I'd vote to remove this one and point people to use getCurrentResult, which gets you a similar result but is synchronous.
There was a problem hiding this comment.
I you think something could be deprecated, then do it. Especially if there already is an alternative. Now is a good time to simplify the API. And if users actually report it as an issue, it can always be un-deprecated or re-implemented in a better way.
The only internal use I found it in ObservableQuery.setVariables(), and that should be replaceable with getCurrentResult().
There was a problem hiding this comment.
Good to know! I tagged most of these in comments as a discussion point for @phryneas. Definitely hoping we can slim down some of these.
Most of these that track some kind of "current" value will likely disappear (e.g. getLast, getLastError, etc.). Now that we're using a BehaviorSubject, we'd like to use that to report the current value via getCurrentResult (which also gives us object stability unlike v3). That said, getCurrentResult will be updated to report only what was last emitted, so there will be some subtle differences with v3 (for example getCurrentResult won't report a loading state if notifyOnNetworkStatusChange is false). I'm hoping that is ok on your end.
There was a problem hiding this comment.
I've never used getCurrentResult. But I am pretty sure BehaviorSubject is the way to go if you want to keep a way to get that kind of data.
My personal use of @apollo/client is a surprisingly small API surface. Out of curiosity I made some stats on what I use. Numbers are not that much representative. For instance, we do a lot more querying that mutating, but querying are typically done via some custom wrapper, so the number of calls shown here is artificially much lower than reality.
The most notable thing is that we almost don't use any API of ObservableQuery. Early on we found out that working with setVariables() and refetch() was not working as expected (#2712 and #3692). So we came up with a custom wrapper that take observable variables and switchMap() to watchQuery(), essentially re-creating an entirely new ObservableQuery every time the variables change and avoiding all pitfalls of setVariables() and refetch().
This might be an unusual way to work with Apollo ( 🤔 ). So you may want to take my opinion about Apollo API with a grain of salt. But it's been working nicely for us for the past 7 years.
(PS: One thing I should do in the future is to work more with fragment...)
| API | Calls | % |
|---|---|---|
ApolloClient.cache |
12 | 3.38% |
ApolloClient.clearStore() |
1 | 0.28% |
ApolloClient.getObservableQueries() |
1 | 0.28% |
ApolloClient.mutate() |
141 | 39.72% |
ApolloClient.query() |
105 | 29.58% |
ApolloClient.reFetchObservableQueries() |
21 | 5.92% |
ApolloClient.resetStore() |
28 | 7.89% |
ApolloClient.watchQuery() |
24 | 6.76% |
ApolloClient.writeQuery() |
9 | 2.54% |
InMemoryCache.evict() |
4 | 1.13% |
InMemoryCache.gc() |
3 | 0.85% |
InMemoryCache.identify() |
2 | 0.56% |
InMemoryCache.readFragment() |
1 | 0.28% |
InMemoryCache.writeQuery() |
1 | 0.28% |
ObservableQuery.refetch() |
2 | 0.56% |
There was a problem hiding this comment.
Yeah, all for removing methods that we don't see an immediate useland usecase for and that we don't use ourselves or could easily replace. The api interface of ObservableQuery has grown unneccesarily complex, so let's reduce it when we can.
There was a problem hiding this comment.
essentially re-creating an entirely new ObservableQuery every time the variables change
@PowerKiKi its interesting you say this because I've noodled on doing this as well. In fact, this is how useSuspenseQuery (and our other suspense hooks) work. We create new ObservableQuery instances each time variables change.
Making ObservableQuery locked to a specific set of variables resonates with me and something I would actually love to do as it reduces a lot of internal complexity (albeit with a slightly more complex end-user experience if you're using the core API directly). That said, its a big change and one we likely won't do for v4. Good to have some validation though on your end about this and we will certainly weigh that behavior change at some point.
There was a problem hiding this comment.
I think for us the key thing was to have observable variables. Once we decided upon that, it was trivial to add a switchMap (and debounceTime for good measure). I feel if you are working with rxjs anyway, then switchMap is not that complex for end-users to grasp.
In fact the old apollo-angular used to have an implementation of observable variables. Then it got removed (not exactly sure why). And then people wanted to have it back. That old thread never got anywhere really. But I suppose that if it was added to apollo-client, it might be a popular-ish feature, assuming users are making more use of rxjs everywhere...
But all of this is out of topic 😇
| } | ||
|
|
||
| // TODO: Consider deprecating this function | ||
| public getLastResult( |
There was a problem hiding this comment.
Eventually I'd like getCurrentResult, getLast*, etc. to just return the last value emitted from the observable (hence why its a BehaviorSubject). That will be done separately as that has some fairly major implications to the returned value from getCurrentResult. Regardless, once that is done, there really isn't much need for tracking the last result since its already stored in the subject.
| // request. As soon as someone observes the query, the request will kick | ||
| // off. For now, we just store any changes. (See #1077) | ||
| return this.observers.size ? this.result() : Promise.resolve(); | ||
| return this.hasObservers() ? this.result() : Promise.resolve(); |
There was a problem hiding this comment.
Drawing attention to this. This behavior in the comment is technically true of any methods that call reobserve, but for some reason setVariables is the only one that provides this guard.
I'd instead opt to make this behavior a bit more explicit. We should consider not allowing refetch, fetchMore, etc if a user hasn't subscribed to the query yet since there is no observable (pun intended) effect besides maybe writing to the cache (depending on the fetch policy). That, or we should just remove this check from setVariables and allow this to fetch the same way if refetch were called without first subscribing.
#1077 seems to agree with this thinking. I'd just like to make that case for all methods that use reobserve under the hood 🙂
There was a problem hiding this comment.
I'd say we have a semantic difference here:
setVariablessets variables. Refetching is only a consequence of "we want data for this" and "variables changed". There are already explicit documented cases when a refetch will not happen (comment above - variables don't actually change, no refetch).refetch,fetchMore, etc. are named to imperatively do that action. There's no wiggle room.
So to me it feels like this return here kinda makes sense. What doesn't make sense anymore is the whole code block, including the comment. It suggests that in some situations when variables didn't change, there would actually be a refetch, although the method is clearly documented as not doing so.
To me, it feels like setVariables should always just resolve with the last result (and never with a void promise!) if the variables didn't change.
| public reobserveAsConcast( | ||
| // TODO: catch `EmptyError` and rethrow as network error if `complete` | ||
| // notification is emitted without a value. | ||
| public reobserve( |
There was a problem hiding this comment.
Another big change with this PR that isn't quite so obvious is that we now let requests initiated by reobserve complete even if all subscribers to ObservableQuery unsubscribe. This is done as a result of using lastValueFrom which provides a separate subscription to the observable that completes once the underlying link observable completes.
The whole reason we had the split of reobserveAsConcast and reobserve was for useLazyQuery which allowed requests kicked off by the execute function to complete. Now that this behavior is part of core, we can unify this function again 🎉
| ); | ||
| } | ||
|
|
||
| public resubscribeAfterError( |
There was a problem hiding this comment.
Now that we don't emit error events when the link observable errors, we have no need for this function since the observable remains active after an error 🎉
| .promise as TODO; | ||
| return lastValueFrom( | ||
| this.fetchObservableWithInfo(queryId, options, networkStatus).observable, | ||
| { defaultValue: undefined } |
There was a problem hiding this comment.
This is legacy behavior from Concast which would resolve with nothing if there were no sources. I've left this as-is for now, but we'll probably want to revisit this at some point.
| options.context, | ||
| options.variables | ||
| ).pipe( | ||
| catchError((networkError) => { |
There was a problem hiding this comment.
I moved catchError above the map here to keep this similar to how asyncMap worked, which handled errors from the source observable, but not errors from the map portion.
| // TODO: Revisit when trying to understand why this passes on release-4.0 | ||
| // branch since client.subscribe returns an observable that isn't | ||
| // subscribed to in this test | ||
| // ["subscription", { method: "subscribe", option: "query" }], |
There was a problem hiding this comment.
Note to self: I believe the reason this worked before is because getObservableFromLink used a Concast which eagerly processed the source observables rather than waiting for an active subscription.
| partial: false, | ||
| }); | ||
|
|
||
| await expect(stream).toEmitApolloQueryResult({ |
There was a problem hiding this comment.
Now that link errors don't terminate the observable query, we actually get a new result emitted from the observable after the refetch.
| expect(result).toEqualApolloQueryResult({ | ||
| data: { | ||
| counter: 4, | ||
| counter: 5, |
There was a problem hiding this comment.
The update in count here makes senses for a couple reasons:
- New values are emitted after errors due to the new behavior, which means the error result above allows the result to continue execution
- This query uses a
cache-and-networkfetch policy, so the counter is called both for the "cache" and the "network" parts of the request.
| // 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 🎉
5 participants
Closes #5295
Closes #12183
Fixes #12349
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