[Compiler Bug]: Lint plugin is too strict about passing hooks as values

[devongovett]

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)
• [X] 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://github.com/adobe/react-spectrum/issues/7220

Repro steps

In React Aria, we have several instances where we use dependency injection to reduce bundle size impact and allow global configuration.

• RouterProvider accepts a useHref prop that allows globally configuring all links in an app. Usually you pass a value directly imported from another library like react-router-dom, which should be safe (the function never changes).
• Our implementation of drag and drop is dependency injected to reduce bundle size. Rather than building the entire drag and drop implementation (which can be quite large) into all components that support it and bloating their bundle size even if you aren't using drag and drop, the drag and drop hooks are injected only if you actually use it. Again, since the actual function implementation of these hooks are static and never change, this can be done safely.

The new lint plugin now causes errors in these cases for users of our library (see https://github.com/adobe/react-spectrum/issues/7220), which I believe to be safe. The documentation describes a few reasons why this might be avoided in regular app code, such as debuggability and locality of behavior, but does not describe any reason why this would actually break anything. Swapping to a different function at runtime would be one such case, but if you don't do that I think it is safe.

Is it possible to loosen this rule so that the value of the hook that is passed into a component is checked to determine if it is static? For example, if you pass down a function in module scope, or something imported from another module, this should be safe since the value never changes, but if you pass down an unmemoized callback declared within a component as a hook this would not be allowed.

How often does this bug happen?

Every time

What version of React are you using?

N/A

What version of React Compiler are you using?

N/A

[josephsavona]

Hey @devongovett, thanks for filing such a detailed issue!

RouterProvider accepts a useHref prop that allows globally configuring all links in an app. Usually you pass a value directly imported from another library like react-router-dom, which should be safe (the function never changes).

It's really tempting to say that it's fine to pass a "dynamic" hook as long as the value is actually constant. Here's a counterexample where that breaks down:

import useHrefFoo from '...;
import useHrefBar from '...';

function Parent({condition}) {
  return (
    <Child 
      condition={condition}
      item={<Provider useHref={useHrefFoo} />} // constant value, "fine"!
      other={<Provider useHref={useHrefBar} />} /> // constant value, "fine"!
  );
}
function Child({condition, item, other}) {
  // Statically looks fine - no obvious dynamic hooks here.
  // *but* - the two <Provider> instances above can reconcile here, 
  // such that the `useHref` prop actually becomes reactive on `condition`
  return condition ? item : other; 
}
function Provider({useHref}) {
  return useHref(); // "fine" because all usages of Provider are supposed to pass a constant value
}

Here, the issue is that even though the individual Provider useHref values are constant, they can end up reconciling together such that the actual useHref value of the provider instance becomes dynamic/reactive.

That all said, we could potentially create some heuristics that we could apply to try to figure when a JSX element likely won't ever need to reconcile and it would be safe to allow a constant value. But I'm curious to understand more of the constraints for why you ended up with the props-based dependency injection approach.

[devongovett]

Yeah, it's not perfect. JS is so dynamic heh. 😄 We do throw runtime errors if the value changes, just in case someone does something silly.

I'm not sure exactly how we'd change the API to avoid all usage of dependency injection though. It's a very useful tool for some cases like these, and works well as long as you are careful about following the rules. Just hard to prove statically that you are doing so.

In the router case, I guess users would have to manually create wrappers around every component that can be a link, e.g. MenuItem, ComboBoxItem, TableRow, etc. and call useHref from their router themselves there:

function MyMenuItem(props) {
  let href = useHref(props.href);
  return <ReactAriaMenuItem {...props} href={href} />
}

That adds some overhead vs configuring all links globally.

The drag and drop case is harder. In that case we want to inject the internal hooks into the bundle only when drag and drop is actually used, since it adds a relatively significant amount of code that might not always be used. Another alternative we explored a few years ago was to create a whole separate set of components, so we'd have two copies of each component that supports drag and drop. ListBox and DraggableListBox, ListBoxItem and DraggableListBoxItem, Table and DraggableTable, Row and DraggableRow, etc. It felt like a lot.

Not only would that mean more components to document, it would mean adding drag and drop to an existing component would be a much larger refector for users than just adding a dragAndDropHooks prop. And that work would propagate out to all the libraries using React Aria Components as well, e.g. each design system would need to have duplicates of each of these components too.

So I'm not sure what we'd do here. The compiler is interesting because the rules need to be enforced more strictly to avoid breaking things. I'm ok with using eslint-disable inside our library and implementing manual memoization where the compiler bails out, but when that propagates out into public API it becomes harder to justify.

[josephsavona]

The drag and drop case is harder. In that case we want to inject the internal hooks into the bundle only when drag and drop is actually used, since it adds a relatively significant amount of code that might not always be used

Can you provide a repro of the drag and drop case? From looking at the link you provided, it seems like you wouldn't get an error except within React Aria's code itself. See an example - note that the error is within ListBox (part of React Aria), not within the consuming code. As you noted it seems fine for a library to occasionally have to bypass rules (ie, to take on the extra burden of ensuring safety) so long as users of the library don't have to violate the rules.

This also suggests a way around the useHref case - basically, if you use a proxy object instead, it would bypass the validation:

import MyUseHref from '...';

function Route() {
  const hrefProvider = useHrefProvider(MyUseHref); // may not have to be a hook
  return <Provider hrefProvider={hrefProvider} />
}

[devongovett]

It seems like you wouldn't get an error except within React Aria's code itself

Yes, that's true in this case. If ignoring it inside React Aria is the answer I'm ok with it.

This also suggests a way around the useHref case

Yeah that seems reasonable for our API, but I users might still have the same problem in their own code. For example react-router-dom exports useHref, and this would error:

import {useHref} from 'react-router-dom';

const hrefProvider = useHrefProvider(useHref);

[josephsavona]

but I users might still have the same problem in their own code. For example react-router-dom exports useHref, and this would error

The challenge is that this example looks just like other cases that are legit errors. There are some things we could change to make this case easier — we could change the way React reconciles, for example, and then allow constant-value hooks as props for JSX that provably can't reconcile with anything else — but those have their own tradeoffs. We think the current rules are clear and generally easy to follow. Per the drag-and-drop example, it's also possible to design an API such that the unsafety lives in the library and not the consumer. In this context, "unsafe" isn't a bad word, it's just about setting expectations.

[devongovett]

Yeah, the challenge is when the API crosses library boundaries, such as in this case, where react-router only exports a hook, not something designed to be passed into a component. So in this case we'd have to have an eslint-disable comment in our documentation example for users to copy into their code, which feels not great. We can do that if there's no other way.