[eslint-plugin-react-hooks] invariants, conditional calls and exhaustive deps

[eps1lon]

Do you want to request a feature or report a bug?
Not sure. Either feature to allow invariant conditional calls to hooks or bug in exhaustive deps in which case #14920 might be more appropriate.

What is the current behavior?

function makeStyles(stylesObjectOrCreator) {
  const listenToTheme = typeof stylesObjectOrCreator === "function";
  const noopTheme = {};

  return function useStyles() {
    const theme = listenToTheme ? React.useContext(ThemeContext) : noopTheme;
    //                           ^^^ [eslint] [...] is called conditionally

    const styles = React.useMemo(
      () => {
        if (listenToTheme) {
          console.log("listen");
          return stylesObjectOrCreator(theme);
        }
        console.log("dont listen");
        return stylesObjectOrCreator;
      },
      [theme]
    );

    const classNames = useStylesheet(styles);
    return classNames;
  };
}

Linter reports that React.useContext is called conditionally but not that listenToTheme is missing in the dependency list.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem. Your bug will get fixed much faster if we can run your code and it doesn't have dependencies other than React. Paste the link to your JSFiddle (https://jsfiddle.net/Luktwrdm/) or CodeSandbox (https://codesandbox.io/s/new) example below:

https://codesandbox.io/s/o91zl0z499

What is the expected behavior?

It would expect one of these things:

1. rules-of-hooks recognizes that listenToTheme is invariant in the hook. Invariant conditions do not trigger called conditionally.
2. exhaustive-deps reports that listenToTheme is missing

It's IMO more dangerous to add an eslint-disable-next-line to invariant conditional calls because it might be missed if someone changes the condition so that it no longer is invariant.

It only occurs to me now that you don't consider it invariant because one might use it in the following way:

function VariantComponent({ listen }) {
  // `listen` controles React.useContext call :(
  const classes = makeStyles(listen ? () => ({}) : {})();
}

Still doesn't explain the missing exhaustive-deps warning.

Are you open to a rule exception via configuration like allow-invariant-conditionals? Users would need to add additional rules so that hook factories are not called inside components.

Ok talking to myself now: Even if everything stays the same and I remove the conditional call then users can still break their app by calling hook factories inside components:

const makeState = (initial) => () => React.useState(initial);
function Component() {
  const usedState = Math.random() < .5 ? makeState(5)() : null;
  // conditional hook call that's undetected
}

[Jessidhia]

const useTheme = listenToTheme
  ? () => React.useContext(ThemeContext)
  : () => noopTheme
const noopTheme = {}

return function useStyles() {
  const theme = useTheme()
  // ...
}

[eps1lon]

That looks likes a good pattern for hook factories, thanks. Maybe something worth adding to the docs if hook factories are common enough?

[oliviertassinari]

I'm not sure it's better than:

function makeStyles(stylesObjectOrCreator) {
  const listenToTheme = typeof stylesObjectOrCreator === "function";
  const noopTheme = {};

  return function useStyles() {
    // eslint-disable-next-line react-hooks/*
    const theme = listenToTheme ? React.useContext(ThemeContext) : noopTheme;

[eps1lon]

@oliviertassinari What makes the previous pattern better is that you're not able to use variables in the condition that are declared in the useStyles scope. The condition is already evaluated and you can't cause any damage to it by changing the hook function body.

To check that we're not calling the hook factory in the component we can use the approach you suggested:

function makeStyles(styles) {
  if (process.env.NODE_ENV !== "production") {
    let isInsideComponent = true;
    try {
      React.useState();
    } catch (err) {
      isInsideComponent = false;
    }

    if (isInsideComponent) {
      console.warn(
        `it appears youre calling makeStyles('${styles}') inside a component`
      );
    }
  }

  return function useStyles() {
    const [state, setState] = React.useState();

    return state;
  };
}

https://codesandbox.io/s/108xlo9m93

[MatthewHerbst]

I think this is a bug report, but maybe there is a reason why it's not?

useEffect(
    () => {
        props.loadCalendar(
            governmentId,
            {
                endDate: currentRange[1].toISOString(),
                startDate: currentRange[0].toISOString(),
            }
        );
    },
    [currentRange, governmentId, props.loadCalendar]
);

This gives the error/warning:

React Hook useEffect has a missing dependency: 'props'. Either include it or remove the dependency array.eslint(react-hooks/exhaustive-deps)

If I destructure loadCalendar above the useEffect and just call it directly there is no issue:

const {
    loadCalendar,
} = props;

useEffect(
    () => {
        loadCalendar(
            governmentId,
            {
                endDate: currentRange[1].toISOString(),
                startDate: currentRange[0].toISOString(),
            }
        );
    },
    [currentRange, governmentId, loadCalendar]
);

AFAIK, props.loadCalendar and loadCalendar are the same thing here, and changes to other parts of props should have no impact on the hook.

[hamlim]

@MatthewHerbst The reason the rule suggests props here is because it adds a this context to the props.loadCalendar call in your first effect. Which means that the loadCalendar method may depend on other values in props implicitly so the rule errs on the side of suggesting putting props in the dependency array.

See here for additional context: #14920 (comment)

[eps1lon]

@MatthewHerbst did you mean to post this in #14920?

[MatthewHerbst]

@eps1lon @hamlim answered my question. It's hard to know exactly which thread to post what in - had thought about just making an issue for it but there seemed to be context in these.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution.

[stale]

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!