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][rfc] Enable hook guards in dev mode by default #32524

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mofeiZ
Copy link
Contributor

@mofeiZ mofeiZ commented Mar 5, 2025

This validation ensures that React compiler-enabled apps remain correct. That is, code that errors with this validation is most likely invalid with React compiler is enabled (specifically, hook calls will be compiled to if-else memo blocks).

Hook guards are used extensively for Meta's react compiler rollouts. There, they're enabled for developers (for dev builds) and on e2e test runs. Let's enable by default for oss as well

Examples of inputs this rule throws on

  • Components should not be invoked directly as React Compiler could memoize the call to AnotherComponent, which introduces conditional hook calls in its compiled output.

    function Invalid1(props) {
     const myJsx = AnotherComponent(props);
     return <div> { myJsx } </div>;
    }
  • Hooks must be named as hooks. Similarly, hook calls may not appear in functions that are not components or hooks.

    const renamedHook = useState;
    function Invalid2() {
      const [state, setState] = renamedHook(0);
    }
    
    function Invalid3() {
      const myFunc = () => useContext(...);
      myFunc();
    }
  • Hooks must be directly called (from the body of a component or hook)

    function call(fn) {
      return fn();
    }
    
    function Invalid4() {
      const result = call(useMyHook);
    }
    

Example of hook guard error (in dev build)

image

This validation ensures that React compiler-enabled apps remain correct. That is, code that errors with this validation is most likely ***invalid*** with React compiler is enabled (specifically, hook calls will be compiled to if-else memo blocks).

Hook guards are used extensively for Meta's react compiler rollouts. There, they're enabled for developers (for dev builds) and on e2e test runs. Let's enable by default for oss as well

### Examples of inputs this rule throws on

* Components should not be invoked directly as React Compiler could memoize the call to AnotherComponent, which introduces conditional hook calls in its compiled output.
  ```js
  function Invalid1(props) {
   const myJsx = AnotherComponent(props);
   return <div> { myJsx } </div>;
  }
  ```
* Hooks must be named as hooks. Similarly, hook calls may not appear in functions that are not components or hooks.
  ```js
  const renamedHook = useState;
  function Invalid2() {
    const [state, setState] = renamedHook(0);
  }

  function Invalid3() {
    const myFunc = () => useContext(...);
    myFunc();
  }
  ```

* Hooks must be directly called (from the body of a component or hook)
  ```
  function call(fn) {
    return fn();
  }

  function Invalid4() {
    const result = call(useMyHook);
  }
  ```


### Example of hook guard error (in dev build)
<img width="1237" alt="image" src="https://github.com/user-attachments/assets/e9ada403-b0d7-4840-b6d5-ad600519c6e6" />
@@ -50,6 +50,7 @@
"pretty-format": "^24",
"react": "0.0.0-experimental-4beb1fd8-20241118",
"react-dom": "0.0.0-experimental-4beb1fd8-20241118",
"react-compiler-runtime": "0.0.1",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We might also need some way of ensuring react-compiler-runtime is bundled (if any bundlers do pre-babel treeshaking).

@mofeiZ mofeiZ requested a review from poteto March 5, 2025 20:14
Copy link
Member

@poteto poteto left a comment

Choose a reason for hiding this comment

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

Hmm, I think it makes sense to have more validation but getting this rolled out will be tricky because we haven't asked people to install react-compiler-runtime this whole time, so this shouldn't break people's apps today if they upgrade the compiler if they don't happen to already have this package installed.

We could maybe update the compiler docs to ask that this package be also installed, and have the babel plugin check if it's there before this option is enabled, and if not it could emit a warning for now and continue.

I think we should also tweak the error message to make it clearer what to do: what about "A hook (useState) was called unexpectedly but React Compiler expected a plain function call. Check that all hooks are called directly, and are named according to convention ("use[A-Z]")"

@josephsavona
Copy link
Contributor

josephsavona commented Mar 6, 2025

First off: I agree that it makes sense for this to be on by default in development in order to help find invalid patterns. Certainly it makes sense to enable for the beta/RC and get feedback on it.

Seconding @poteto's concern: the idea for react-compiler-runtime was that you'd only have to install it if you were using an older version of React than the one the compiler was designed for. With this change we're just requiring everyone to install the runtime package, which isn't great. I think we should consider moving the runtime pieces required for hook guards into the main react package. Then we could align the compiler RC with the canary of the react release w those features, the stable release with the stable react release including those necessary features, etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants