Skip to content

Commit

Permalink
RenderPromises: use canonicalStringify to serialize data, use `Tr…
Browse files Browse the repository at this point in the history
…ie` (#11799)

* `RenderPromises`: use `canonicalStringify` to serialize data, use `Trie`
fixes #11798
This ensures that queries with variables of equal contents, but different order, will be handled the same way during `renderToStringWithData` SSR.
It also replaces a hand-written Trie implementation with the `Trie` dependency.

* Update .changeset/early-pots-rule.md

Co-authored-by: Jerel Miller <jerelmiller@gmail.com>

---------

Co-authored-by: Jerel Miller <jerelmiller@gmail.com>
  • Loading branch information
phryneas and jerelmiller committed Apr 24, 2024
1 parent 60592e9 commit 1aca7ed
Show file tree
Hide file tree
Showing 3 changed files with 71 additions and 16 deletions.
5 changes: 5 additions & 0 deletions .changeset/early-pots-rule.md
@@ -0,0 +1,5 @@
---
"@apollo/client": patch
---

`RenderPromises`: use `canonicalStringify` to serialize `variables` to ensure query deduplication is properly applied even when `variables` are specified in a different order.
27 changes: 13 additions & 14 deletions src/react/ssr/RenderPromises.ts
@@ -1,8 +1,9 @@
import type { DocumentNode } from "graphql";
import type * as ReactTypes from "react";

import type { ObservableQuery, OperationVariables } from "../../core/index.js";
import type { QueryDataOptions } from "../types/types.js";
import { Trie } from "@wry/trie";
import { canonicalStringify } from "../../cache/index.js";

// TODO: A vestigial interface from when hooks were implemented with utility
// classes, which should be deleted in the future.
Expand All @@ -16,11 +17,13 @@ type QueryInfo = {
observable: ObservableQuery<any, any> | null;
};

function makeDefaultQueryInfo(): QueryInfo {
return {
function makeQueryInfoTrie() {
// these Tries are very short-lived, so we don't need to worry about making it
// "weak" - it's easier to test and debug as a strong Trie.
return new Trie<QueryInfo>(false, () => ({
seen: false,
observable: null,
};
}));
}

export class RenderPromises {
Expand All @@ -31,13 +34,13 @@ export class RenderPromises {
// objects. These QueryInfo objects are intended to survive through the whole
// getMarkupFromTree process, whereas specific Query instances do not survive
// beyond a single call to renderToStaticMarkup.
private queryInfoTrie = new Map<DocumentNode, Map<string, QueryInfo>>();
private queryInfoTrie = makeQueryInfoTrie();

private stopped = false;
public stop() {
if (!this.stopped) {
this.queryPromises.clear();
this.queryInfoTrie.clear();
this.queryInfoTrie = makeQueryInfoTrie();
this.stopped = true;
}
}
Expand Down Expand Up @@ -133,13 +136,9 @@ export class RenderPromises {
private lookupQueryInfo<TData, TVariables extends OperationVariables>(
props: QueryDataOptions<TData, TVariables>
): QueryInfo {
const { queryInfoTrie } = this;
const { query, variables } = props;
const varMap = queryInfoTrie.get(query) || new Map<string, QueryInfo>();
if (!queryInfoTrie.has(query)) queryInfoTrie.set(query, varMap);
const variablesString = JSON.stringify(variables);
const info = varMap.get(variablesString) || makeDefaultQueryInfo();
if (!varMap.has(variablesString)) varMap.set(variablesString, info);
return info;
return this.queryInfoTrie.lookup(
props.query,
canonicalStringify(props.variables)
);
}
}
55 changes: 53 additions & 2 deletions src/react/ssr/__tests__/useQuery.test.tsx
Expand Up @@ -2,12 +2,17 @@
import React from "react";
import { DocumentNode } from "graphql";
import gql from "graphql-tag";
import { MockedProvider, mockSingleLink } from "../../../testing";
import {
MockedProvider,
MockedResponse,
mockSingleLink,
} from "../../../testing";
import { ApolloClient } from "../../../core";
import { InMemoryCache } from "../../../cache";
import { ApolloProvider } from "../../context";
import { ApolloProvider, getApolloContext } from "../../context";
import { useApolloClient, useQuery } from "../../hooks";
import { renderToStringWithData } from "..";
import type { Trie } from "@wry/trie";

describe("useQuery Hook SSR", () => {
const CAR_QUERY: DocumentNode = gql`
Expand Down Expand Up @@ -333,4 +338,50 @@ describe("useQuery Hook SSR", () => {
expect(cache.extract()).toMatchSnapshot();
});
});

it("should deduplicate `variables` with identical content, but different order", async () => {
const mocks: MockedResponse[] = [
{
request: {
query: CAR_QUERY,
variables: { foo: "a", bar: 1 },
},
result: { data: CAR_RESULT_DATA },
maxUsageCount: 1,
},
];

let trie: Trie<any> | undefined;
const Component = ({
variables,
}: {
variables: { foo: string; bar: number };
}) => {
const { loading, data } = useQuery(CAR_QUERY, { variables, ssr: true });
trie ||=
React.useContext(getApolloContext()).renderPromises!["queryInfoTrie"];
if (!loading) {
expect(data).toEqual(CAR_RESULT_DATA);
const { make, model, vin } = data.cars[0];
return (
<div>
{make}, {model}, {vin}
</div>
);
}
return null;
};

await renderToStringWithData(
<MockedProvider mocks={mocks}>
<>
<Component variables={{ foo: "a", bar: 1 }} />
<Component variables={{ bar: 1, foo: "a" }} />
</>
</MockedProvider>
);
expect(
Array.from(trie!["getChildTrie"](CAR_QUERY)["strong"].keys())
).toEqual(['{"bar":1,"foo":"a"}']);
});
});

0 comments on commit 1aca7ed

Please sign in to comment.