Bug: Eslint hooks returned by factory functions not linted

[KATT]

React version: *

Steps To Reproduce

See my draft PR with failing tests 👉 #25066.

Given the following code:

// Factory function for creating hooks
function createHooks() {
  return {
    foo: {
      useQuery: () => {
        data: 'foo.useQuery'
      },
    }
  };
}
const hooks = createHooks();

export const MyComponent = () => {
  if (Math.random() < 0.5) {
    return null;
  }
  // ❌ This should fail the linter
  const query = hooks.foo.useQuery();

  return <>{query.data}</>;
}

The current behavior

The linting does not catch that the hooks.foo.useQuery() is used conditionally.

The expected behavior

The linting should catch that the hooks.foo.useQuery() is used conditionally.

Failing tests / link to code

See my draft PR with failing tests 👉 #25066.

I've highlighted areas and things that are up for discussions around this.

Additional context

Partial workaround

If the object returned is not a deep getter, it's possible to PascalCase it and do it like this:

// Factory function for creating hooks
function createHooks() {
  return {
    useFoo: () => {
      data: 'foo.useQuery'
  };
}
const Hooks = createHooks();

export const MyComponent = () => {
  if (Math.random() < 0.5) {
    return null;
  }
  // ✅ This will fail the linter
  const query = Hooks.useFoo();

  return <>{query.data}</>;
}

It's okay if hooks can be called outside of React-components

#25065 (comment)

Background

I'm the creator of tRPC where we use the following pattern for users to create the root hooks:

// Initialization of the typesafe tRPC hooks
export const trpc = createReactQueryHooks<AppRouter>();

// MyComponent.tsx
export function MyComponent() {
  const query = trpc.useQuery(['post.byId', { id: '1' }])

  return <pre>{JSON.stringify(query.data ?? null, null, 4)}</pre>
}

In the coming version of tRPC, we are planning on have an API that looks like the below, which also won't be caught.

export function MyComponent() {
  const query = trpc.post.byId.useQury({ id: '1'});

  return <pre>{JSON.stringify(query.data ?? null, null, 4)}</pre>
}

[KATT]

We're very happy to do a PR to change the behavior to suit our needs, but before doing so I'd love some takes & constraints from the React team so we don't do a bunch of work in vain that might be rejected regardless.

[phryneas]

This also potentially impacts RTK Query.

While most of our users probably use

export const { useAddUserQuery } = api

// in another file
const result = useAddUserQuery(arg)

it is also valid to just import api and use (especially with old TS versions that don't support template literals)

const result = api.endpoints.addUser.useQuery()

and while I have never noticed it, it seems like this would also not count as a hook for the eslint rules then?

[eps1lon]

I understand that people want this to count as a hook but also consider the larger ecosystem impact. jest.useFakeTimers is an often used API that would start triggering this rule once any namespace is considered. The same may apply to other existing APIs.

but before doing so I'd love some takes & constraints from the React team so we don't do a bunch of work in vain that might be rejected regardless.

I would strongly encourage you to do so regardless. PRs aren't predicated on the fact that they will get merged. They're after all, a request. A PR will allow trying out the rule in existing codebase to find potential false-positives and false-negatives. Otherwise we continue to rely on each others biases on what we think is important without having an actual real-word picture: testing something in production.

[phryneas]

I understand that people want this to count as a hook but also consider the larger ecosystem impact. jest.useFakeTimers is an often used API that would start triggering this rule once any namespace is considered. The same may apply to other existing APIs.

To your knowledge, are there any such apis that are actively being called within React components? (useFakeTimers would only be called in tests, not components)

Maybe there could be a middle ground that would apply only some rules of hooks for nested use calls - obviously not the "hooks must be called in a component" rule - that one would break with your example and also has a runtime warning that triggers reliably anyways.

[eps1lon]

To your knowledge, are there any such apis that are actively being called within React components? (useFakeTimers would only be called in tests, not components)

So you'd be ok with api.endpoints.addUser.useQuery() being called outside of a component without the lint rule catching it? This is a bit different from the original post. Now we're talking just about calling the hook conditionally not generally being linted for. So api.endpoints.addUser.useQuery() being called anywhere outside of a component would be ok for you?

[phryneas]

So api.endpoints.addUser.useQuery() being called anywhere outside of a component would be ok for you?

Personally: Yes, because it would in 100% of the cases produce a runtime error that would pop up during development. It's nice to lint for that, but it's easy to detect and easy to fix.

Conditionally calling a hook in a component is something that might take a lot of time to actually surface and produce a runtime error during development though - so here the lint rule adds a lot more value.

---

Another completely different idea: eslint rules can be type aware - why not make use of that and allow library authors to mark functions as "yes, this is a hook and the rules of hooks should apply"?
Maybe by adding a (in the types only, not affecting runtime) { [thisIsAReactHookSymbol]: true } property to the hook function.

[eps1lon]

Another completely different idea: eslint rules can be type aware - why not make use of that and allow library authors to mark functions as "yes, this is a hook and the rules of hooks should apply"?

This would require type-aware linting which is super slow and definitely not viable at the current state of the ecosystem. It would also require lint rules that understand flow and would break with untyped code.

Personally: Yes, because it would in 100% of the cases produce a runtime error that would pop up during development. It's nice to lint for that, but it's easy to detect and easy to fix.

If @KATT is ok with that then I would suggest updating the issue description so that we're all on the same page.

[TkDodo]

Maybe there could be a middle ground that would apply only some rules of hooks for nested use calls - obviously not the "hooks must be called in a component" rule - that one would break with your example and also has a runtime warning that triggers reliably anyways.

This seems like a good idea to me. The problem with the conditional hook call not being flagged is that it might be introduced silently by adding an early return to an already existing component. Then, you might not get the "fewer hooks have been rendered" error until you actually render that branch that does return early. So here, the lint rule is very important to work reliably (even if there might be some false positives).

[KATT]

Sorry, I dropped the ball on this until someone reminded me that this was not solved.

Personally: Yes, because it would in 100% of the cases produce a runtime error that would pop up during development. It's nice to lint for that, but it's easy to detect and easy to fix.

If @KATT is ok with that then I would suggest updating the issue description so that we're all on the same page.

Yes, this would be fine for me too. I've updated the description.

---

Is there anything we can do to enable this behaviour? Would you accept a PR?