From 7fb7939edb7ca8f4273b75554f96ea9936731458 Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Wed, 12 Jun 2024 13:14:10 +0200 Subject: [PATCH] switch `useRenderGuard` to an approach not accessing React's internals (#11888) --- .changeset/brown-bikes-divide.md | 5 ++ .size-limits.json | 2 +- .../__tests__/useRenderGuard.test.tsx | 43 -------------- src/react/hooks/internal/useRenderGuard.ts | 56 +++++++++++++------ 4 files changed, 45 insertions(+), 61 deletions(-) create mode 100644 .changeset/brown-bikes-divide.md diff --git a/.changeset/brown-bikes-divide.md b/.changeset/brown-bikes-divide.md new file mode 100644 index 00000000000..8701c21eae9 --- /dev/null +++ b/.changeset/brown-bikes-divide.md @@ -0,0 +1,5 @@ +--- +"@apollo/client": patch +--- + +switch `useRenderGuard` to an approach not accessing React's internals diff --git a/.size-limits.json b/.size-limits.json index 4176d307cbb..5c28672b1f7 100644 --- a/.size-limits.json +++ b/.size-limits.json @@ -1,4 +1,4 @@ { - "dist/apollo-client.min.cjs": 39620, + "dist/apollo-client.min.cjs": 39561, "import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 32821 } diff --git a/src/react/hooks/internal/__tests__/useRenderGuard.test.tsx b/src/react/hooks/internal/__tests__/useRenderGuard.test.tsx index ff27fb82a3c..0f60cb58892 100644 --- a/src/react/hooks/internal/__tests__/useRenderGuard.test.tsx +++ b/src/react/hooks/internal/__tests__/useRenderGuard.test.tsx @@ -2,7 +2,6 @@ import React, { useEffect } from "rehackt"; import { useRenderGuard } from "../useRenderGuard"; import { render, waitFor } from "@testing-library/react"; -import { withCleanup } from "../../../../testing/internal"; const UNDEF = {}; const IS_REACT_19 = React.version.startsWith("19"); @@ -35,45 +34,3 @@ it("returns a function that returns `false` if called after render", async () => }); expect(result).toBe(false); }); - -function breakReactInternalsTemporarily() { - const R = React as unknown as { - __SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED: any; - }; - const orig = R.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED; - - R.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED = {}; - return withCleanup({}, () => { - R.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED = orig; - }); -} - -it("results in false negatives if React internals change", () => { - let result: boolean | typeof UNDEF = UNDEF; - function TestComponent() { - using _ = breakReactInternalsTemporarily(); - const calledDuringRender = useRenderGuard(); - result = calledDuringRender(); - return <>Test; - } - render(); - expect(result).toBe(false); -}); - -it("does not result in false positives if React internals change", async () => { - let result: boolean | typeof UNDEF = UNDEF; - function TestComponent() { - using _ = breakReactInternalsTemporarily(); - const calledDuringRender = useRenderGuard(); - useEffect(() => { - using _ = breakReactInternalsTemporarily(); - result = calledDuringRender(); - }); - return <>Test; - } - render(); - await waitFor(() => { - expect(result).not.toBe(UNDEF); - }); - expect(result).toBe(false); -}); diff --git a/src/react/hooks/internal/useRenderGuard.ts b/src/react/hooks/internal/useRenderGuard.ts index 2d5a798fc3c..ba101f109b7 100644 --- a/src/react/hooks/internal/useRenderGuard.ts +++ b/src/react/hooks/internal/useRenderGuard.ts @@ -1,23 +1,45 @@ import * as React from "rehackt"; -function getRenderDispatcher() { - return (React as any).__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED - ?.ReactCurrentDispatcher?.current; -} - -let RenderDispatcher: unknown = null; +let Ctx: React.Context; -/* -Relay does this too, so we hope this is safe. -https://github.com/facebook/relay/blob/8651fbca19adbfbb79af7a3bc40834d105fd7747/packages/react-relay/relay-hooks/loadQuery.js#L90-L98 -*/ +function noop() {} export function useRenderGuard() { - // eslint-disable-next-line react-compiler/react-compiler - RenderDispatcher = getRenderDispatcher(); + if (!Ctx) { + // we want the intialization to be lazy because `createContext` would error on import in a RSC + Ctx = React.createContext(null); + } + + return React.useCallback( + /** + * @returns true if the hook was called during render + */ () => { + const orig = console.error; + try { + console.error = noop; - return React.useCallback(() => { - return ( - RenderDispatcher != null && RenderDispatcher === getRenderDispatcher() - ); - }, []); + /** + * `useContext` can be called conditionally during render, so this is safe. + * (Also, during render we would want to throw as a reaction to this anyways, so it + * wouldn't even matter if we got the order of hooks mixed up...) + * + * They cannot however be called outside of Render, and that's what we're testing here. + * + * Different versions of React have different behaviour on an invalid hook call: + * + * React 16.8 - 17: throws an error + * https://github.com/facebook/react/blob/2b93d686e359c7afa299e2ec5cf63160a32a1155/packages/react/src/ReactHooks.js#L18-L26 + * + * React 18 & 19: `console.error` in development, then `resolveDispatcher` returns `null` and a member access on `null` throws. + * https://github.com/facebook/react/blob/58e8304483ebfadd02a295339b5e9a989ac98c6e/packages/react/src/ReactHooks.js#L28-L35 + */ + React["useContext" /* hide this from the linter */](Ctx); + return true; + } catch (e) { + return false; + } finally { + console.error = orig; + } + }, + [] + ); }