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

linter: followups for react/exhaustive-dep #7246

Open
camc314 opened this issue Nov 12, 2024 · 3 comments
Open

linter: followups for react/exhaustive-dep #7246

camc314 opened this issue Nov 12, 2024 · 3 comments
Assignees

Comments

@camc314
Copy link
Contributor

camc314 commented Nov 12, 2024

Following from #7151, there are a few changes left to complete for parity with reacts lint rule:

  1. unknown dependeencies should report on the useCallback ident, not the second arg fix(linter): react/exhaustive-deps update span for unknown deps diagnostic #7249
  ⚠ react_hooks(exhaustive-deps): React Hook useCallback received a function whose dependencies are unknown.
    ╭─[kibana/src/plugins/console/public/application/hooks/use_save_current_text_object.ts:27:5]
 26 │       return useCallback(
 27 │ ╭─▶     throttle(
 28 │ │         (text: string) => {
 29 │ │           const { current: promise } = promiseChainRef;
 30 │ │           if (!currentTextObject) return;
 31 │ │           promise.finally(() =>
 32 │ │             objectStorageClient.text.update({ ...currentTextObject, text, updatedAt: Date.now() })
 33 │ │           );
 34 │ │         },
 35 │ │         WAIT_MS,
 36 │ │         { trailing: true }
 37 │ ╰─▶     ),
 38 │         [objectStorageClient, currentTextObject]
    ╰────
  help: Pass an inline function instead.
  1. custom nodes should be supported
  2. ensure that the following disabled cases pass/fail as expected: fix(linter): fix false positives in exhaustive-deps #7615
function useMyThing(myRef) {
    useEffect(() => {
        const handleMove = () => {};
        myRef.current = {};
        return () => {
            console.log(myRef.current.toString());
        };
    }, [myRef]);
}

In the below case, we need to mark props as a dependency because it has a this value. #7615

function MyComponent(props) {
    const [skillsCount] = useState();
    useEffect(() => {
        if (skillsCount === 0 && !props.isEditMode) {
            props.toggleEditMode();
        }
    }, [skillsCount, props.isEditMode, props.toggleEditMode]);
}

I think we can just ignore the below case. It will throw an error (JS error) if you try to execute it (foo is used before its declaration)

function Example() {
    const foo = useCallback(() => {
        foo();
    }, [foo]);
}

Same as above case

function Example({ prop }) {
    const foo = useCallback(() => {
        prop.hello(foo);
    }, [foo]);
    const bar = useCallback(() => {
        foo();
    }, [foo]);
}
  1. we should report the name (excluding the function name. fix(linter): fix false positives in exhaustive-deps #7615
    currently
function MyComponent(props) {
          useCallback(() => {
            console.log(props.foo?.toString());
          }, []);
        }

produces an error saying props.foo.toString should be a dep.
this should be changed to say that props.foo should be a dep.

@camc314
Copy link
Contributor Author

camc314 commented Dec 3, 2024

#7626

https://github.com/getsentry/sentry/blob/e6e6ded544348df69b5f43c40154d7860df4eaec/static/app/components/profiling/flamegraph/interactions/useCanvasZoomOrScroll.tsx#L52

  × eslint-plugin-react-hooks(exhaustive-deps): The ref's value `.current` is accessed directly in the effect cleanup function.
    ╭─[static/app/components/profiling/flamegraph/interactions/useCanvasZoomOrScroll.tsx:53:37]
 52 │       if (wheelStopTimeoutId.current !== undefined) {
 53 │         window.cancelAnimationFrame(wheelStopTimeoutId.current);
    ·                                     ──────────────────────────
 54 │       }
    ╰────
  help: The ref value will likely have changed by the time this effect cleanup function runs. If this ref points to a node rendered by react, copy it to a variable inside the effect and use that variable in the cleanup function.
  × eslint-plugin-react-hooks(exhaustive-deps): The ref's value `.current` is accessed directly in the effect cleanup function.
    ╭─[static/app/components/profiling/flamegraph/interactions/useCanvasZoomOrScroll.tsx:52:11]
 51 │     return () => {
 52 │       if (wheelStopTimeoutId.current !== undefined) {
    ·           ──────────────────────────
 53 │         window.cancelAnimationFrame(wheelStopTimeoutId.current);
    ╰────
  help: The ref value will likely have changed by the time this effect cleanup function runs. If this ref points to a node rendered by react, copy it to a variable inside the effect and use that variable in the cleanup function.

@camc314
Copy link
Contributor Author

camc314 commented Dec 3, 2024

https://github.com/getsentry/sentry/blob/98aa6bf95c3af9e5cf6a3477bac7aa2bc78ace82/static/app/views/sentryAppExternalInstallation/index.tsx#L125

  × eslint-plugin-react-hooks(exhaustive-deps): React Hook useEffect contains a call to setState. Without a list of dependencies, this can lead to an infinite chain of updates.
     ╭─[static/app/views/sentryAppExternalInstallation/index.tsx:125:3]
 124 │ 
 125 │   useEffect(function () {
     ·   ─────────
 126 │     // Skip if we have a selected org, or if there aren't any orgs loaded yet.
     ╰────
  help: Consider adding an empty list of dependencies to make it clear which values are intended to be stable.

@camc314
Copy link
Contributor Author

camc314 commented Dec 3, 2024

export const But = (props) => {
    let container;
    const [containerRect, setContainerRect] = useState<DOMRect | null>(null);
    const portalOffset = 5;

    useEffect(() => {
        container = document.getElementById('foo');
        if (container) {
            setContainerRect(container.getBoundingClientRect());
        }
    }, []);

    return (
        <div />
    );
};
  × eslint-plugin-react-hooks(exhaustive-deps): React Hook useEffect has a missing dependency: 'container'
     ╭─[<path>.tsx:205:8]
 204 │         }
 205 │     }, []);
     ·        ──
 206 │ 
     ╰────
  help: Either include it or remove the dependency array.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant