From d98b87b493f5edaba594c5dfd285cc57f01fb8f0 Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Thu, 4 Apr 2024 11:44:07 +0200 Subject: [PATCH] 0.10.0 (2) - Warn if makeClient returns the same client for multiple requests. (#205) * Warn if makeClient returns the same client for multiple requests. * Add test * Apply suggestions from code review Co-authored-by: Jerel Miller --------- Co-authored-by: Jerel Miller --- .../src/registerApolloClient.test.tsx | 64 +++++++++++++++++++ .../src/registerApolloClient.tsx | 47 ++++++++++++-- 2 files changed, 106 insertions(+), 5 deletions(-) diff --git a/packages/client-react-streaming/src/registerApolloClient.test.tsx b/packages/client-react-streaming/src/registerApolloClient.test.tsx index 37e834e6..a78c6a7d 100644 --- a/packages/client-react-streaming/src/registerApolloClient.test.tsx +++ b/packages/client-react-streaming/src/registerApolloClient.test.tsx @@ -1,3 +1,4 @@ +/* eslint-disable no-inner-declarations */ import { it } from "node:test"; import assert from "node:assert"; import { runInConditions } from "./util/runInConditions.js"; @@ -129,3 +130,66 @@ it("calling `getClient` with parameters results in an error", async () => { /You cannot pass arguments into `getClient`./.test(error?.message || "") ); }); + +it("warns if `makeClient` calls that should return different `ApolloClient` instances return the same one (outside of React)", () => { + const warn = console.warn; + try { + let warnArgs: unknown[] | undefined = undefined; + console.warn = (...args) => (warnArgs = args); + const same = makeClient(); + const { getClient } = registerApolloClient(() => same); + + getClient(); + // `console.warn` has not been called + assert.equal(warnArgs, undefined); + + getClient(); + // `console.warn` has been called + assert.ok( + /Multiple calls to `getClient` for different requests returned the same client instance./i.test( + String(warnArgs![0]) + ) + ); + } finally { + console.warn = warn; + } +}); + +it("warns if `makeClient` calls that should return different `ApolloClient` instances return the same one (in React)", async () => { + const warn = console.warn; + try { + let warnArgs: unknown[] | undefined = undefined; + console.warn = (...args) => (warnArgs = args); + const same = makeClient(); + const { getClient } = registerApolloClient(() => same); + + function App() { + getClient(); + // it should be perfectly fine and not cause any errors to call `getClient` in here as often as we want to + getClient(); + getClient(); + return
; + } + + { + const stream = renderToPipeableStream(React.createElement(App), {}); + await drain(stream); + } + // we had only one request, `console.warn` has not been called + assert.equal(warnArgs, undefined); + + { + // second render - different request, but returns `same` ApolloClient as before + const stream = renderToPipeableStream(React.createElement(App), {}); + await drain(stream); + } + + assert.ok( + /Multiple calls to `getClient` for different requests returned the same client instance./i.test( + String(warnArgs![0]) + ) + ); + } finally { + console.warn = warn; + } +}); diff --git a/packages/client-react-streaming/src/registerApolloClient.tsx b/packages/client-react-streaming/src/registerApolloClient.tsx index 28ad6a70..977c6d57 100644 --- a/packages/client-react-streaming/src/registerApolloClient.tsx +++ b/packages/client-react-streaming/src/registerApolloClient.tsx @@ -1,10 +1,17 @@ import type { ApolloClient } from "@apollo/client/index.js"; import { cache } from "react"; +const seenWrappers = WeakSet + ? new WeakSet<{ client: ApolloClient | Promise> }>() + : undefined; +const seenClients = WeakSet + ? new WeakSet | Promise>>() + : undefined; + /** * > This export is only available in React Server Components * - * Ensures that you can always access the same instance of ApolloClient + * Ensures that you can always access the same instance of ApolloClient * during RSC for an ongoing request, while always returning * a new instance for different requests. * @@ -26,7 +33,7 @@ export function registerApolloClient( makeClient: () => Promise> ): { getClient: () => Promise> }; /** - * Ensures that you can always access the same instance of ApolloClient + * Ensures that you can always access the same instance of ApolloClient * during RSC for an ongoing request, while always returning * a new instance for different requests. * @@ -50,7 +57,19 @@ export function registerApolloClient(makeClient: () => ApolloClient): { export function registerApolloClient( makeClient: (() => Promise>) | (() => ApolloClient) ) { - function wrappedMakeClient() { + // React invalidates the cache on each server request, so the wrapping + // object is needed to properly detect whether the client is a unique + // reference or not. We can warn if `cachedMakeWrappedClient` creates a new "wrapper", + // but with a `client` property that we have already seen before. + // In that case, not every call to `makeClient` would create a new + // `ApolloClient` instance. + function makeWrappedClient() { + return { client: makeClient() }; + } + + const cachedMakeWrappedClient = cache(makeWrappedClient); + + function getClient() { if (arguments.length) { throw new Error( ` @@ -61,9 +80,27 @@ resulting in duplicate requests and a non-functional cache. `.trim() ); } - return makeClient(); + const wrapper = cachedMakeWrappedClient(); + if (seenWrappers && seenClients) { + if (!seenWrappers.has(wrapper)) { + if (seenClients.has(wrapper.client)) { + console.warn( + ` +Multiple calls to \`getClient\` for different requests returned the same client instance. +This means that private user data could accidentally be shared between requests. +This happens, for example, if you create a global \`ApolloClient\` instance and your \`makeClient\` +implementation just looks like \`() => client\`. +Always call \`new ApolloClient\` **inside** your \`makeClient\` function and +return a new instance every time \`makeClient\` is called. +`.trim() + ); + } + seenWrappers.add(wrapper); + seenClients.add(wrapper.client); + } + } + return wrapper.client; } - const getClient = cache(wrappedMakeClient); return { getClient, };