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

[Compiler]: React Compiler should error on document.getElementById call during render #31932

Open
4 tasks
rishitells opened this issue Dec 29, 2024 · 3 comments
Open
4 tasks

Comments

@rishitells
Copy link

rishitells commented Dec 29, 2024

What kind of issue is this?

  • React Compiler core (the JS output is incorrect, or your app works incorrectly after optimization)
  • babel-plugin-react-compiler (build issue installing or using the Babel plugin)
  • eslint-plugin-react-compiler (build issue installing or using the eslint plugin)
  • react-compiler-healthcheck (build issue installing or using the healthcheck script)

Link to repro

https://playground.react.dev/#N4Igzg9grgTgxgUxALhAMygOzgFwJYSYAEAIngIYA2EA5gOp44AWAChDDlQBTBF5gB5AA4JiAXwCURYAB1iROITA4FhTnkwIYRALxEcATxEQ0RACYQ4UALaiVAQh16ZILGYRoNCMy6IB+c0sbOwA6GgQcAFFKBFtMHAAhAwBJMy4AciF2TkoAWhgICBx0qWQiTChKSgBuOSIiGAjYYi46+qIAHjIqWiIIEUwdYH5hUTEAPjb2zu7qGhC2DipVePIvGCHFVfWJqemZijmQgGE1O3GACQQqiA6AelnaE7P4yflp+8f5xZy3j4fDrQ-kQJLVMGIQGIgA

Repro steps

Steps to reproduce

  1. Create a simple dialog component that uses Portal with a dynamic container:
function DialogWithPortal({ isOpen }) {
  const container = typeof document !== "undefined" ? document.getElementById('portal-root') : null;
  return (
    <Dialog open={isOpen}>
      <Dialog.Portal container={container}>
        <Dialog.Content>Hello</Dialog.Content>
      </Dialog.Portal>
    </Dialog>
  );
}
  1. Mount the component when portal container exists in DOM:
// DOM has: <div id="portal-root"></div>
<DialogWithPortal isOpen={true} />
  1. Observe that dialog doesn't open because compiler memoizes initial null container value

Expected behavior:

  • document.getElementById() should be evaluated on each render to find the portal container

Actual behaviour:

  • Compiler memoizes the initial document.getElementById() call
  • If container doesn't exist on first render, null is cached permanently
  • Subsequent renders use cached null value even after container is added to DOM

The issue is in how the compiler transforms the code:

// Initial DOM query is memoized and never rechecked
  let t1;
  if ($[0] === Symbol.for("react.memo_cache_sentinel")) {
    t1 =
      typeof document !== "undefined"
        ? document.getElementById("portal-root")
        : null;
    $[0] = t1;
  } else {
    t1 = $[0];
  }
  const container = t1;

How often does this bug happen?

Every time

What version of React are you using?

^18.2.0

What version of React Compiler are you using?

19.0.0-beta-201e55d-20241215

@rishitells rishitells added Component: Optimizing Compiler Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug Type: Bug labels Dec 29, 2024
@rishitells rishitells changed the title [Compiler Bug]: React Compiler incorrectly memoizes document.getElementById calls causing Portals to fail [Compiler Bug]: React Compiler incorrectly memoizes document.getElementById call Dec 29, 2024
@rishitells rishitells changed the title [Compiler Bug]: React Compiler incorrectly memoizes document.getElementById call [Compiler Bug]: React Compiler incorrectly memoizes document.getElementById call during render Dec 29, 2024
@josephsavona
Copy link
Contributor

Thanks for posting! Reading external mutable values during render is against the rules in React: components should render the same result given the same props, state, and context. getElementById() accesses external mutable state - the DOM - which can change w/o React being aware. Read more here: https://react.dev/reference/rules#components-and-hooks-must-be-pure

Instead, React supports features like refs to get a reference to a DOM node, or useEffect to run side effects (where it is safe to access external mutable state). I would recommend moving the access of the element into a useEffect call, and storing that current value in a ref (from useRef()).

The compiler tries to detect invalid code like in the example and skip compilation, but we can’t detect all possible patterns. In this case we could consider looking for getElementById() calls and bailing out (w an InvalidReact error). Cc @poteto

@josephsavona josephsavona removed Type: Bug Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug labels Dec 29, 2024
@josephsavona josephsavona changed the title [Compiler Bug]: React Compiler incorrectly memoizes document.getElementById call during render [Compiler]: React Compiler should error on document.getElementById call during render Dec 29, 2024
@rishitells
Copy link
Author

rishitells commented Dec 30, 2024

Thank you so much for the detailed explanation, @josephsavona! You're absolutely right - this is a pure function violation and I should have known better. I somehow missed that getElementById is accessing external mutable state here.

What made me report this was actually the silent failure - I was expecting the compiler to either bail out or show an error. I completely understand that detecting all possible patterns across all platforms (web, native) isn't feasible. I'm curious though - would detecting web-specific APIs like common DOM queries (getElementById, querySelector) create too tight of a coupling, or would there be a way to conditionally run this kind of validation based on platform? Probably having the ESLint plugin warn about such usages could be enough?

I'm currently working on adopting the React compiler in the content form of Hygraph CMS, and while it's been working great overall (except for a few React Hook Form issues I've already solved), this particular case silently broke our form functionality. Having the compiler, or the ESLint plugin warn about these common pitfalls would be super helpful for the adoption journey.

@josephsavona
Copy link
Contributor

I've started the validation in #31960

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants