Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hide queryRef in a Symbol in useBackgroundQuery return value #11010

Merged
merged 5 commits into from
Jun 26, 2023
Merged

Hide queryRef in a Symbol in useBackgroundQuery return value #11010

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ccab651
chore: hide queryRef in a Symbol (closes #10875)
alessbell Jun 23, 2023
c775f3e
chore: add changeset
alessbell Jun 23, 2023
561ccab
chore: prettier fixes
alessbell Jun 23, 2023
35b7667
fix: rename useReadQuery parameter to queryRef
alessbell Jun 26, 2023
d2c2379
chore: increase size limit
alessbell Jun 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/smooth-clouds-sort.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@apollo/client': patch
---

Hide queryRef in a Symbol in `useBackgroundQuery`s return value.
4 changes: 2 additions & 2 deletions .size-limit.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
const checks = [
{
path: "dist/apollo-client.min.cjs",
limit: "37700"
limit: "37750"
},
{
path: "dist/main.cjs",
Expand All @@ -10,7 +10,7 @@ const checks = [
{
path: "dist/index.js",
import: "{ ApolloClient, InMemoryCache, HttpLink }",
limit: "33269"
limit: "33375"
},
...[
"ApolloProvider",
Expand Down
17 changes: 14 additions & 3 deletions src/react/cache/QueryReference.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,20 +9,31 @@ import { NetworkStatus, isNetworkRequestSettled } from '../../core';
import type { ObservableSubscription } from '../../utilities';
import { createFulfilledPromise, createRejectedPromise } from '../../utilities';
import type { CacheKey } from './types';
import type { useBackgroundQuery, useReadQuery } from '../hooks';

type Listener<TData> = (promise: Promise<ApolloQueryResult<TData>>) => void;

type FetchMoreOptions<TData> = Parameters<
ObservableQuery<TData>['fetchMore']
>[0];

interface QueryReferenceOptions {
export const QUERY_REFERENCE_SYMBOL: unique symbol = Symbol();
/**
* A `QueryReference` is an opaque object returned by {@link useBackgroundQuery}.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open to suggestions/wordsmithing here! I'm not wedded to this language, but wanted something short and that gets at the notion that QueryReference should only be passed into useReadQuery/its properties should not be accessed directly.

* A child component reading the `QueryReference` via {@link useReadQuery} will
* suspend until the promise resolves.
*/
export interface QueryReference<TData = unknown> {
[QUERY_REFERENCE_SYMBOL]: InternalQueryReference<TData>;
}

interface InternalQueryReferenceOptions {
key: CacheKey;
onDispose?: () => void;
autoDisposeTimeoutMs?: number;
}

export class QueryReference<TData = unknown> {
export class InternalQueryReference<TData = unknown> {
public result: ApolloQueryResult<TData>;
public readonly key: CacheKey;
public readonly observable: ObservableQuery<TData>;
Expand All @@ -41,7 +52,7 @@ export class QueryReference<TData = unknown> {

constructor(
observable: ObservableQuery<TData>,
options: QueryReferenceOptions
options: InternalQueryReferenceOptions
) {
this.listen = this.listen.bind(this);
this.handleNext = this.handleNext.bind(this);
Expand Down
8 changes: 4 additions & 4 deletions src/react/cache/SuspenseCache.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { Trie } from '@wry/trie';
import type { ObservableQuery } from '../../core';
import { canUseWeakMap } from '../../utilities';
import { QueryReference } from './QueryReference';
import { InternalQueryReference } from './QueryReference';
import type { CacheKey } from './types';

interface SuspenseCacheOptions {
Expand All @@ -19,7 +19,7 @@ interface SuspenseCacheOptions {
}

export class SuspenseCache {
private queryRefs = new Trie<{ current?: QueryReference }>(canUseWeakMap);
private queryRefs = new Trie<{ current?: InternalQueryReference }>(canUseWeakMap);
private options: SuspenseCacheOptions;

constructor(options: SuspenseCacheOptions = Object.create(null)) {
Expand All @@ -33,7 +33,7 @@ export class SuspenseCache {
const ref = this.queryRefs.lookupArray(cacheKey);

if (!ref.current) {
ref.current = new QueryReference(createObservable(), {
ref.current = new InternalQueryReference(createObservable(), {
key: cacheKey,
autoDisposeTimeoutMs: this.options.autoDisposeTimeoutMs,
onDispose: () => {
Expand All @@ -42,6 +42,6 @@ export class SuspenseCache {
});
}

return ref.current as QueryReference<TData>;
return ref.current as InternalQueryReference<TData>;
}
}
21 changes: 11 additions & 10 deletions src/react/hooks/__tests__/useBackgroundQuery.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import {
import { concatPagination, offsetLimitPagination } from '../../../utilities';
import { useBackgroundQuery, useReadQuery } from '../useBackgroundQuery';
import { ApolloProvider } from '../../context';
import { QUERY_REFERENCE_SYMBOL } from '../../cache/QueryReference';
import { SuspenseCache } from '../../cache';
import { InMemoryCache } from '../../../cache';
import {
Expand Down Expand Up @@ -617,7 +618,7 @@ describe('useBackgroundQuery', () => {

const [queryRef] = result.current;

const _result = await queryRef.promise;
const _result = await queryRef[QUERY_REFERENCE_SYMBOL].promise;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A note to anyone who'd argue these tests are testing implementation details: I'd agree! The vast majority are integration tests, though we also have a few unit tests that violate this rule in a way that feels like an acceptable tradeoff to keep some tests more compact :)


expect(_result).toEqual({
data: { hello: 'world 1' },
Expand Down Expand Up @@ -654,7 +655,7 @@ describe('useBackgroundQuery', () => {

const [queryRef] = result.current;

const _result = await queryRef.promise;
const _result = await queryRef[QUERY_REFERENCE_SYMBOL].promise;

await waitFor(() => {
expect(_result).toEqual({
Expand Down Expand Up @@ -739,7 +740,7 @@ describe('useBackgroundQuery', () => {

const [queryRef] = result.current;

const _result = await queryRef.promise;
const _result = await queryRef[QUERY_REFERENCE_SYMBOL].promise;

await waitFor(() => {
expect(_result).toMatchObject({
Expand Down Expand Up @@ -803,7 +804,7 @@ describe('useBackgroundQuery', () => {

const [queryRef] = result.current;

const _result = await queryRef.promise;
const _result = await queryRef[QUERY_REFERENCE_SYMBOL].promise;
const resultSet = new Set(_result.data.results);
const values = Array.from(resultSet).map((item) => item.value);

Expand Down Expand Up @@ -868,7 +869,7 @@ describe('useBackgroundQuery', () => {

const [queryRef] = result.current;

const _result = await queryRef.promise;
const _result = await queryRef[QUERY_REFERENCE_SYMBOL].promise;
const resultSet = new Set(_result.data.results);
const values = Array.from(resultSet).map((item) => item.value);

Expand Down Expand Up @@ -913,7 +914,7 @@ describe('useBackgroundQuery', () => {

const [queryRef] = result.current;

const _result = await queryRef.promise;
const _result = await queryRef[QUERY_REFERENCE_SYMBOL].promise;

expect(_result).toEqual({
data: { hello: 'from link' },
Expand Down Expand Up @@ -956,7 +957,7 @@ describe('useBackgroundQuery', () => {

const [queryRef] = result.current;

const _result = await queryRef.promise;
const _result = await queryRef[QUERY_REFERENCE_SYMBOL].promise;

expect(_result).toEqual({
data: { hello: 'from cache' },
Expand Down Expand Up @@ -1006,7 +1007,7 @@ describe('useBackgroundQuery', () => {

const [queryRef] = result.current;

const _result = await queryRef.promise;
const _result = await queryRef[QUERY_REFERENCE_SYMBOL].promise;

expect(_result).toEqual({
data: { foo: 'bar', hello: 'from link' },
Expand Down Expand Up @@ -1049,7 +1050,7 @@ describe('useBackgroundQuery', () => {

const [queryRef] = result.current;

const _result = await queryRef.promise;
const _result = await queryRef[QUERY_REFERENCE_SYMBOL].promise;

expect(_result).toEqual({
data: { hello: 'from link' },
Expand Down Expand Up @@ -1095,7 +1096,7 @@ describe('useBackgroundQuery', () => {

const [queryRef] = result.current;

const _result = await queryRef.promise;
const _result = await queryRef[QUERY_REFERENCE_SYMBOL].promise;

expect(_result).toEqual({
data: { hello: 'from link' },
Expand Down
29 changes: 16 additions & 13 deletions src/react/hooks/useBackgroundQuery.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,10 @@ import type {
} from '../../core';
import { NetworkStatus } from '../../core';
import { useApolloClient } from './useApolloClient';
import type { QueryReference } from '../cache/QueryReference';
import {
QUERY_REFERENCE_SYMBOL,
type QueryReference,
} from '../cache/QueryReference';
import type { SuspenseQueryHookOptions, NoInfer } from '../types/types';
import { __use } from './internal';
import { useSuspenseCache } from './useSuspenseCache';
Expand Down Expand Up @@ -188,7 +191,7 @@ export function useBackgroundQuery<

return useMemo(() => {
return [
queryRef,
{ [QUERY_REFERENCE_SYMBOL]: queryRef },
{
fetchMore,
refetch,
Expand All @@ -199,41 +202,41 @@ export function useBackgroundQuery<

export function useReadQuery<TData>(queryRef: QueryReference<TData>) {
const [, forceUpdate] = useState(0);

const internalQueryRef = queryRef[QUERY_REFERENCE_SYMBOL];
invariant(
queryRef.promiseCache,
internalQueryRef.promiseCache,
'It appears that `useReadQuery` was used outside of `useBackgroundQuery`. ' +
'`useReadQuery` is only supported for use with `useBackgroundQuery`. ' +
'Please ensure you are passing the `queryRef` returned from `useBackgroundQuery`.'
);

const skipResult = useMemo(() => {
const error = toApolloError(queryRef.result);
const error = toApolloError(internalQueryRef.result);

return {
loading: false,
data: queryRef.result.data,
data: internalQueryRef.result.data,
networkStatus: error ? NetworkStatus.error : NetworkStatus.ready,
error,
};
}, [queryRef.result]);
}, [internalQueryRef.result]);

let promise = queryRef.promiseCache.get(queryRef.key);
let promise = internalQueryRef.promiseCache.get(internalQueryRef.key);

if (!promise) {
promise = queryRef.promise;
queryRef.promiseCache.set(queryRef.key, promise);
promise = internalQueryRef.promise;
internalQueryRef.promiseCache.set(internalQueryRef.key, promise);
}

useEffect(() => {
return queryRef.listen((promise) => {
queryRef.promiseCache!.set(queryRef.key, promise);
return internalQueryRef.listen((promise) => {
internalQueryRef.promiseCache!.set(internalQueryRef.key, promise);
forceUpdate((prevState) => prevState + 1);
});
}, [queryRef]);

const result =
queryRef.watchQueryOptions.fetchPolicy === 'standby'
internalQueryRef.watchQueryOptions.fetchPolicy === 'standby'
? skipResult
: __use(promise);

Expand Down
6 changes: 3 additions & 3 deletions src/react/hooks/useSuspenseQuery.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import type {
} from '../types/types';
import { useDeepMemo, useStrictModeSafeCleanupEffect, __use } from './internal';
import { useSuspenseCache } from './useSuspenseCache';
import type { QueryReference } from '../cache/QueryReference';
import type { InternalQueryReference } from '../cache/QueryReference';
import { canonicalStringify } from '../../cache';

export interface UseSuspenseQueryResult<
Expand Down Expand Up @@ -296,8 +296,8 @@ export function toApolloError(result: ApolloQueryResult<any>) {
: result.error;
}

export function useTrackedQueryRefs(queryRef: QueryReference) {
const trackedQueryRefs = useRef(new Set<QueryReference>());
export function useTrackedQueryRefs(queryRef: InternalQueryReference) {
const trackedQueryRefs = useRef(new Set<InternalQueryReference>());

trackedQueryRefs.current.add(queryRef);

Expand Down