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.

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

POC reproduction of Issue #9204 #9407

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
86 changes: 85 additions & 1 deletion src/react/hooks/__tests__/useQuery.test.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, { Fragment } from 'react';
import React, { Fragment, useEffect, useState } from 'react';
import { DocumentNode, GraphQLError } from 'graphql';
import gql from 'graphql-tag';
import { act } from 'react-dom/test-utils';
Expand All @@ -20,6 +20,90 @@ import { useQuery } from '../useQuery';
import { useMutation } from '../useMutation';

describe('useQuery Hook', () => {
describe('regression test issue #9204', () => {
const Component = ({ query }: any) => {
const [counter, setCounter] = useState(0)
const result = useQuery(query)

useEffect(() => {
/**
* IF the return value from useQuery changes on each render,
* this component will re-render in an infinite loop.
*/
console.log('CURRENT RESULT CHANGED')
setCounter(p => p + 1)
}, [
result
])

if (result.loading) return null

return <div>{result.data.hello}{counter}</div>
}

itAsync('should handle a simple query', (resolve, reject) => {
const query = gql`{ hello }`;
const mocks = [
{
request: { query },
result: { data: { hello: "world" } },
},
];

const { getByText } = render(
<MockedProvider mocks={mocks}>
<Component query={query} />
</MockedProvider>
);

waitFor(() => {
expect(getByText('world2')).toBeTruthy();
}).then(resolve, reject);
});
})

describe('regression test issue #9204 - destructured', () => {
const Component = ({ query }: any) => {
const [counter, setCounter] = useState(0)
const {data, loading} = useQuery(query)

useEffect(() => {
/**
* IF the return value from useQuery changes on each render,
* this component will re-render in an infinite loop.
*/
console.log('CURRENT RESULT CHANGED')
setCounter(p => p + 1)
}, [
data
])

if (loading) return null

return <div>{data.hello}{counter}</div>
}

itAsync('should handle a simple query', (resolve, reject) => {
const query = gql`{ hello }`;
const mocks = [
{
request: { query },
result: { data: { hello: "world" } },
},
];

const { getByText } = render(
<MockedProvider mocks={mocks}>
<Component query={query} />
</MockedProvider>
);

waitFor(() => {
expect(getByText('world2')).toBeTruthy();
}).then(resolve, reject);
});
})

describe('General use', () => {
it('should handle a simple query', async () => {
const query = gql`{ hello }`;
Expand Down
23 changes: 13 additions & 10 deletions src/react/hooks/useQuery.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,12 @@ export function useQuery<
const context = useContext(getApolloContext());
const client = useApolloClient(options?.client);
const defaultWatchQueryOptions = client.defaultOptions.watchQuery;
const watchQueryOptions = useMemo(
() => createWatchQueryOptions(query, options, defaultWatchQueryOptions),
[query, options, defaultWatchQueryOptions]
)
Comment on lines +32 to +35
Copy link
Member

Choose a reason for hiding this comment

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

One fun bug: this useMemo works great as long as options is undefined (just useQuery(query)), but as soon as you call useQuery(query, {...}) with options, that's a fresh object for useMemo, so watchQueryOptions gets regenerated with every useMemo call.

verifyDocumentType(query, DocumentType.Query);
const [obsQuery, setObsQuery] = useState(() => {
const watchQueryOptions = createWatchQueryOptions(query, options, defaultWatchQueryOptions);
// See if there is an existing observable that was used to fetch the same
// data and if so, use it instead since it will contain the proper queryId
// to fetch the result set. This is used during SSR.
Expand Down Expand Up @@ -62,7 +65,7 @@ export function useQuery<
{
// The only options which seem to actually be used by the
// RenderPromises class are query and variables.
getOptions: () => createWatchQueryOptions(query, options, defaultWatchQueryOptions),
getOptions: () => watchQueryOptions,
fetchData: () => new Promise<void>((resolve) => {
const sub = obsQuery!.subscribe({
next(result) {
Expand Down Expand Up @@ -108,14 +111,13 @@ export function useQuery<
options,
result,
previousData: void 0 as TData | undefined,
watchQueryOptions: createWatchQueryOptions(query, options, defaultWatchQueryOptions),
watchQueryOptions,
});

// An effect to recreate the obsQuery whenever the client or query changes.
// This effect is also responsible for checking and updating the obsQuery
// options whenever they change.
useEffect(() => {
const watchQueryOptions = createWatchQueryOptions(query, options, defaultWatchQueryOptions);
let nextResult: ApolloQueryResult<TData> | undefined;
if (ref.current.client !== client || !equal(ref.current.query, query)) {
const obsQuery = client.watchQuery(watchQueryOptions);
Expand Down Expand Up @@ -219,8 +221,8 @@ export function useQuery<
return () => subscription.unsubscribe();
}, [obsQuery, context.renderPromises, client.disableNetworkFetches]);

let partial: boolean | undefined;
({ partial, ...result } = result);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have to assume there was a reason for doing this that I don't understand, but it is one of the underlying causes of the loss of identity stability in the return value.

The result of the change here is that result continues to include the partial field, but it could be removed and maintain identity stability via delete result.partial

Copy link
Member

Choose a reason for hiding this comment

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

This got fixed in PR #9459, and now works without rebinding any variables:

const { data, partial, ...resultWithoutPartial } = result;

const partial: boolean | undefined = result.partial;
delete result.partial

{
// BAD BOY CODE BLOCK WHERE WE PUT SIDE-EFFECTS IN THE RENDER FUNCTION
Expand Down Expand Up @@ -252,7 +254,7 @@ export function useQuery<
!options?.skip &&
result.loading
) {
obsQuery.setOptions(createWatchQueryOptions(query, options, defaultWatchQueryOptions)).catch(() => {});
obsQuery.setOptions(watchQueryOptions).catch(() => {});
}

// We assign options during rendering as a guard to make sure that
Expand Down Expand Up @@ -311,14 +313,15 @@ export function useQuery<
subscribeToMore: obsQuery.subscribeToMore.bind(obsQuery),
}), [obsQuery]);

return {
return useMemo(() => ({
...obsQueryFields,
variables: createWatchQueryOptions(query, options, defaultWatchQueryOptions).variables,
variables: watchQueryOptions.variables,
client,
called: true,
previousData: ref.current.previousData,
...result,
};
}), [obsQueryFields, watchQueryOptions.variables, client, ref.current.previousData, result]);

}

/**
Expand Down