react icon indicating copy to clipboard operation
react copied to clipboard

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

Open rishitells opened this issue 1 year ago • 7 comments

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 avatar Dec 29 '24 07:12 rishitells

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 avatar Dec 29 '24 18:12 josephsavona

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.

rishitells avatar Dec 30 '24 04:12 rishitells

I've started the validation in #31960

josephsavona avatar Jan 02 '25 20:01 josephsavona

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

github-actions[bot] avatar Jun 06 '25 19:06 github-actions[bot]

This still needs to be addressed. We can now model impure functions per #31960 but we need to be add a declaration for document.getElementById to tell the compiler that it's impure.

josephsavona avatar Jun 06 '25 19:06 josephsavona

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

github-actions[bot] avatar Sep 05 '25 12:09 github-actions[bot]

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

github-actions[bot] avatar Dec 04 '25 16:12 github-actions[bot]

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you!

github-actions[bot] avatar Dec 11 '25 16:12 github-actions[bot]