Minor improvements to useQuery/ObservableQuery
#12703
Conversation
|
| }); | ||
|
|
||
| // Maintain object identity as much as possible | ||
| return masked === result.data ? result : { ...result, data: masked }; |
There was a problem hiding this comment.
I figured we should try and maintain object identity as much as possible, so it makes sense to return the original result when data hasn't changed.
|
|
||
| default: | ||
| return defaultResult; | ||
| return uninitialized; |
There was a problem hiding this comment.
Since these are a part of getInitialResult, no need to create new objects just to return values deep equal to empty/uninitialized. This better maintains object identity this way.
commit: |
src/react/hooks/useQuery.ts
Outdated
| observable | ||
| ); | ||
| if (!watchQueryOptions.fetchPolicy) { | ||
| watchQueryOptions.fetchPolicy = observable.options.initialFetchPolicy; |
There was a problem hiding this comment.
We already modify watchQueryOptions.fetchPolicy in the if (skip) condition above, so I didn't think it was necessary to have to extract this to its own function just to "use no memo" for the compiler.
There was a problem hiding this comment.
Let's get the compiler merged in before we merge this so we'll notice if something changed there :)
There was a problem hiding this comment.
Yep, I'm ok with that 👍
There was a problem hiding this comment.
Good call on waiting. This caused the compiler to remove all the optimizations so I reverted this back. Looks better now https://github.com/apollographql/apollo-client/actions/runs/15740542099
|
|
||
| useResubscribeIfNecessary<TData, TVariables>( | ||
| resultData, // might get mutated during render | ||
| observable, // might get mutated during render | ||
| watchQueryOptions | ||
| ); | ||
|
|
||
| const resultOverride = useSyncExternalStore( |
There was a problem hiding this comment.
Rather than using a separate useSyncExternalStore for ssr === false, I merged this with the uSES in useResultSubscription.
| } | ||
|
|
||
| function useResultSubscription<TData, TVariables extends OperationVariables>( | ||
| function useResult<TData, TVariables extends OperationVariables>( |
There was a problem hiding this comment.
I shortened the name to save a touch of bundle size.
There was a problem hiding this comment.
I think a minifier should already do that on userland side, but I won't complain :D
| () => resultOverride || resultData.current, | ||
| () => resultOverride || resultData.current | ||
| () => resultData.current, | ||
| () => (ssr === false ? useQuery.ssrDisabledResult : resultData.current) |
There was a problem hiding this comment.
This is the result of merging both uSES together
| return; | ||
| } | ||
|
|
||
| if ( | ||
| previousResult && |
There was a problem hiding this comment.
previousResult is always guaranteed a value as its a non-null type, so this check was unnecessary
jerelmiller
force-pushed
the
jerel/get-emitted-result
branch
from
47d8f5e to
e3365eb
Compare
size-limit report 📦
|
As I was playing around with trying to use
observable.getCurrentResultinsideuseSyncExternalStore, I found a couple places that could use a small bit of improvement. This is the result of those changes which reduces some bundle size in the process.