change tracking behaviour for QueryInfo
#12647
Conversation
🦋 Changeset detectedLatest commit: 6d5c6df The changes in this PR will be included in the next version bump. 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 |
commit: |
size-limit report 📦
|
phryneas
force-pushed
the
pr/adjust-query-tracking
branch
from
ea5945f to
6960e06
Compare
phryneas
changed the base branch from
pr/observablequery-cancel-on-unsubscribe
to
release-4.0
| @@ -4170,47 +4170,6 @@ describe("ApolloClient", () => { | |||
| expect(timesFired).toBe(2); | |||
| }); | |||
|
|
|||
| it("should not error on a stopped query()", async () => { | |||
There was a problem hiding this comment.
After reading #4409 it seems to me that this tests to prevent something that could be achieved in a very old iteration of the Query component or by calling QueryManager functions like stopQuery from userland code - but stopQuery since has been removed and we consider QueryManager an internal API.
=> I think it's fine to remove this test.
.size-limits.json
Outdated
| "import { ApolloClient, InMemoryCache, HttpLink } from \"@apollo/client\" (CJS)": 43087, | ||
| "import { ApolloClient, InMemoryCache, HttpLink } from \"@apollo/client\" (production) (CJS)": 38133, | ||
| "import { ApolloClient, InMemoryCache, HttpLink } from \"@apollo/client\"": 33081, | ||
| "import { ApolloClient, InMemoryCache, HttpLink } from \"@apollo/client\" (production)": 27529 |
There was a problem hiding this comment.
All this good stuff AND a reduction in bundle size 😱. Amazing stuff!
src/core/QueryManager.ts
Outdated
| observer.add(observable.subscribe(observer)); | ||
| observer.add(fetchCancelSubject.subscribe(observer)); |
There was a problem hiding this comment.
| observer.add(observable.subscribe(observer)); | |
| observer.add(fetchCancelSubject.subscribe(observer)); | |
| observable.subscribe(observer); | |
| fetchCancelSubject.subscribe(observer); |
I don't think you actually need the .add here because if you use the observer passed directly to subscribe, it should also handle the teardown. I found this out when implementing deduplicated subscriptions and why I wrote that withRestart function as:
source.subscribe({
next: observer.next.bind(observer),
complete: observer.complete.bind(observer),
error: observer.error.bind(observer),
});If I just passed source.subscribe(observer) I found it was running the cleanup function on my wrapper observable when complete or error was called.
Edit: I just tried this locally and unless we are missing a test somewhere, this seems to work 🙂
There was a problem hiding this comment.
This is even wilder than I thought:
observable.subscribe(observer) internally adds teardown logic and then returns observable.
So observer.add(observable.subscribe(observer)) is actually equivalent to observable.subscribe(observer); observer.add(observer);.
Good catch!
There was a problem hiding this comment.
| * @internal | ||
| * Tears down the `ObservableQuery` and stops all active operations by sending a `complete` notification. | ||
| */ | ||
| public stop() { |
There was a problem hiding this comment.
| public stop() { | |
| public dispose() { |
What are your thoughts on calling this dispose? I think dispose sounds a little more like "I'm done with it".
FWIW I'd also welcome a change on ApolloClient to .dispose instead of stop since the description alludes to this:
/**
* Call this method to terminate any active client processes, making it safe
* to dispose of this `ApolloClient` instance.
*/I'm not opposed to leaving this as stop, but wanted to throw this out there.
There was a problem hiding this comment.
I think I prefer stop - dispose at this point for me hints to Symbol.dispose-like behaviour, and I don't think we would ever want to add a Symbol.dispose property with this functionality to either ApolloClient or ObservableQuery.
It's also less breaking to keep in on AC, and as a result more consistent here.
That said, I had accidentally marked this as @internal when I actually see this being useful in userland - removing the @internal tag.
| */ | ||
| public stop() { | ||
| this.subject.complete(); | ||
| this.initializeObservablesQueue(); |
There was a problem hiding this comment.
This would mean that you could stop an ObservableQuery and then subscribe to it later and continue to receive results correct?
I go back and forth on whether I like this because the complete notification is sent which is generally a signal that the Observable is done emitting values. I know this class is not an Observable itself, but it is observable-like and I think we probably should treat it similar, meaning you'd need to call client.watchQuery again to reinitialize the query. That or we shouldn't call complete so we don't signal that the observable is done emitting values.
Either way, we should probably add a test somewhere to demonstrate what you can/can't do after you call stop on an ObservableQuery then call subscribe again so that we have that documented.
There was a problem hiding this comment.
The problem here is that ObservableQuery blurs the lines between an Observable and (Behavior)Subject a bit - an Observable can be subscribed again after it finishes, since each subscribe event creates and individually running subscription:
test("Observable re-subscribe behaviour after complete", async () => {
const observable = new Observable((observer) => {
observer.next("first");
setTimeout(() => {
observer.next("second");
observer.complete();
}, 20);
});
const stream = new ObservableStream(observable);
await expect(stream).toEmitTypedValue("first");
await expect(stream).toEmitTypedValue("second");
await expect(stream).toComplete();
const stream2 = new ObservableStream(observable);
await expect(stream2).toEmitTypedValue("first");
await expect(stream2).toEmitTypedValue("second");
await expect(stream2).toComplete();
});A Subject on the other hand could not, since all subscribers would get the same subscription:
test("Subject is closed once it's done", async () => {
const subject = new Subject();
setTimeout(() => {
subject.next("first");
});
setTimeout(() => {
subject.next("second");
subject.complete();
}, 50);
const stream = new ObservableStream(subject);
await expect(stream).toEmitTypedValue("first");
await expect(stream).toEmitTypedValue("second");
await expect(stream).toComplete();
const stream2 = new ObservableStream(subject);
await expect(stream2).toComplete();
});So we're a mix of an Observable and BehaviorSubject here - a BehaviorObservable maybe?
Since we have Observable explicitly in the name, I think I prefer this behavior. But good call, I'll add a test.
There was a problem hiding this comment.
Ah ya brain fart on the first point that you can subscribe multiple times. I know this but apparently was super tired. In that case, I like keeping it in! Thanks for adding that test!
Co-authored-by: Jerel Miller <[email protected]>
This reverts commit 5538a2c.
Co-authored-by: Jerel Miller <[email protected]>
Co-authored-by: Jerel Miller <[email protected]>
Co-authored-by: Jerel Miller <[email protected]>
No description provided.