add ignoreResults option to useSubscription
#11921
Conversation
🦋 Changeset detectedLatest commit: 03c9739 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 |
size-limit report 📦
|
✅ Deploy Preview for apollo-client-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| const onData = jest.fn((() => {}) as SubscriptionHookOptions["onData"]); | ||
| const onError = jest.fn((() => {}) as SubscriptionHookOptions["onError"]); | ||
| const onComplete = jest.fn( | ||
| (() => {}) as SubscriptionHookOptions["onComplete"] | ||
| ); |
There was a problem hiding this comment.
| const onData = jest.fn((() => {}) as SubscriptionHookOptions["onData"]); | |
| const onError = jest.fn((() => {}) as SubscriptionHookOptions["onError"]); | |
| const onComplete = jest.fn( | |
| (() => {}) as SubscriptionHookOptions["onComplete"] | |
| ); | |
| const onData = jest.fn(); | |
| const onError = jest.fn(); | |
| const onComplete = jest.fn(); |
Out of curiosity, are these casts needed? I'm able to remove this entirely and see no type errors. If you're using this to convey intent, feel free to ignore this comment.
(this suggestion applies to any place with these mocks, but I'll avoid adding this comment to the other locations)
There was a problem hiding this comment.
It's for test-writing DX. If you hover onData further down in the test to get a feeling what you should assert on, you either get any, or with this the right type.
| expect(onData.mock.lastCall?.[0].data).toStrictEqual({ | ||
| data: results[0].result.data, | ||
| error: undefined, | ||
| loading: false, | ||
| variables: undefined, | ||
| }); |
There was a problem hiding this comment.
| expect(onData.mock.lastCall?.[0].data).toStrictEqual({ | |
| data: results[0].result.data, | |
| error: undefined, | |
| loading: false, | |
| variables: undefined, | |
| }); | |
| expect(onData).toHaveBeenLastCalledWith( | |
| expect.objectContaining({ | |
| data: { | |
| data: results[0].result.data, | |
| error: undefined, | |
| loading: false, | |
| variables: undefined, | |
| }, | |
| }) | |
| ); |
[nit] As a readability thing, it would be great if we could structure this using the expect helpers (toHaveBeenLastCalledWith) rather than trying to directly read from mock directly. I think you were looking to avoid checking against every property passed to that first argument, so perhaps using expect.objectContaining can also get around that. I'm able to get the test to pass the same structuring it this way. This also makes it clear to the reader that there are more properties passed to this argument that we aren't checking as a part of this test 🙂
(this suggestion applies to any location with .mocks.lastCall, so I'll avoid adding this to each place)
There was a problem hiding this comment.
Fair enough - I think I had tried that before, but probably missed something :)
| variables: undefined, | ||
| }); | ||
| // `onData` should not be called again for the same result | ||
| expect(onData).toHaveBeenCalledTimes(1); |
There was a problem hiding this comment.
Could you perhaps add one this assertion before the rerender call as well? This confused me at first because the last time you assert on the number of calls, its checking against 0, so it sort of looks like this function was called after you switch from ignoreResults true -> false. Adding the assertion before rerender makes this obvious that this count didn't increase as a result of rerendering the component and changing the ignoreResults option.
| expect(onData).toHaveBeenCalledTimes(2); | ||
| } | ||
| // a second subscription should not have been started | ||
| expect(subscriptionCreated).toHaveBeenCalledTimes(1); |
| loading: false, | ||
| error: undefined, | ||
| // switching back to the default `ignoreResults: true` return value | ||
| data: undefined, |
There was a problem hiding this comment.
If we return undefined when switching to ignoreResults: true rather than returning the result up to this point, we might want to make this clear in the docs somehow so that someone doesn't expect to continue getting a value here.
FWIW I think this is reasonable behavior since ignoreResults: true is a signifier that you don't care about the value returned from the hook, but I just think adding the extra clarification will help avoid confusion.
Co-authored-by: Jerel Miller <jerelmiller@gmail.com>
…`ignoreResults` later
phryneas
force-pushed
the
pr/useSubscription-ignoreResults
branch
from
d82c489
to
cc41b89
Compare
…ption-ignoreResults
Resolves #10216