Using typechecked typescript-eslint trips `unbound-method` lint on react-stately functions

[jvliwanag]

Provide a general summary of the issue here

Consider the following use of react-stately useOverlayTriggerState:

function MyDialog(): React.ReactNode {
  let { open } = useOverlayTriggerState({}); // lint error here

  return (<div><button onPress={open}/></div>);
}

Typescript-eslint when configured with recommended typechecked config detects a potential error - unbound -method.

🤔 Expected Behavior?

No lint error.

😯 Current Behavior

Lint error unbound-method.

💁 Possible Solution

This is because the way the interface is written is akin to a class method:

export interface OverlayTriggerState {
    readonly isOpen: boolean;
    setOpen(isOpen: boolean): void;
    open(): void;
    close(): void;
    toggle(): void;
}

In fact, if we imagine that useOverlayTriggerState indeed return some class instance, say: new OverlayTriggerStateImpl, destructuring the result in let { open } = useOverlayTriggerState({}) would indeed be erroneous as this wouldn't be properly bound.

Granted, the actual implementation isn't a class and this doesn't really matter. To correct this however, we should be writing the interface as:

export interface OverlayTriggerState {
    readonly isOpen: boolean;
    setOpen: (isOpen: boolean) => void;
    open: () => void;
    close: () => void;
    toggle: () => void;
}

If it were written that way, then the unbound-method lint correctly passes.

🔦 Context

No response

🖥️ Steps to Reproduce

see summary

Version

@react-stately/overlays 3.6.18

What browsers are you seeing the problem on?

Other

If other, please specify.

No response

What operating system are you using?

n/a

🧢 Your Company/Team

No response

🕷 Tracking Issue

No response

[snowystinger]

Thanks for the issue. Is this causing an issue in your project? Trying to understand where the need for this is coming from.

[jvliwanag]

Yes. For users of react stately with typescript eslint enabled and configured with recommended typechecked config, there is a lint rule that’s violated - and a valid one at that given the interface declarations.

As such, i made a pull request that updates the interfaces into a more precise declaration for functions. While doing so as well, it actually helped uncover existing unhandled cases in react stately.

[devongovett]

I think if you just don't destructure the open method it won't be a problem? Just call state.open() instead of open(). I don't think we'd normally recommend destructuring since we have a few methods in our codebase that do use this.

[jvliwanag]

Agree that in places where this matters, we shouldn't change the types. And in those cases, I believe typescript typecheck will guard us anyway.

However, the changes I made are I believe all within useXxx functions anyway and are not classes / using this anyway.

As a bonus, in the pull request I made, the second commit consisted of changes that actually uncovered previously unhandled issues.

If we don't go this route, users of the typechecked config can either:

• Turn off this lint (which would be a shame)
• Transform this:

let { open, close } = useOverlayTriggerState({}); // lint error here

return (<div><button onPress={open}/><button onPress={close}/></div>);

into:

let state = useOverlayTriggerState({});
let open = useCallback(() => state.open(), [state.open]);
let close = useCallback(() => state.close(), [state.close]);

return (<div><button onPress={open}/><button onPress={close}/></div>);

Which, while being correct in the case that OverlayTriggerState is later on implemented as a class where this matters, is quite verbose and has unnecessary memoization.