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

switch useRenderGuard to an approach not accessing React's internals #11888

Merged
merged 2 commits into from
Jun 12, 2024
Merged
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
5 changes: 5 additions & 0 deletions .changeset/brown-bikes-divide.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@apollo/client": patch
---

switch `useRenderGuard` to an approach not accessing React's internals
2 changes: 1 addition & 1 deletion .size-limits.json
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
{
"dist/apollo-client.min.cjs": 39620,
"dist/apollo-client.min.cjs": 39561,
Copy link
Member

Choose a reason for hiding this comment

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

🙌

"import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 32821
}
43 changes: 0 additions & 43 deletions src/react/hooks/internal/__tests__/useRenderGuard.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down Expand Up @@ -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(<TestComponent />);
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(<TestComponent />);
await waitFor(() => {
expect(result).not.toBe(UNDEF);
});
expect(result).toBe(false);
});
56 changes: 39 additions & 17 deletions src/react/hooks/internal/useRenderGuard.ts
Original file line number Diff line number Diff line change
@@ -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<null>;

/*
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;
}
},
[]
);
}
Loading