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 <[email protected]>

---------

Co-authored-by: Jerel Miller <[email protected]>

Author: phryneas

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 <div></div>;
+    }
+
+    {
+      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;
+  }
+});

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<any> | Promise<ApolloClient<any>> }>()
+  : undefined;
+const seenClients = WeakSet
+  ? new WeakSet<ApolloClient<any> | Promise<ApolloClient<any>>>()
+  : 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<ApolloClient<any>>
 ): { getClient: () => Promise<ApolloClient<any>> };
 /**
- * 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<any>): {
 export function registerApolloClient(
   makeClient: (() => Promise<ApolloClient<any>>) | (() => ApolloClient<any>)
 ) {
-  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,
   };