Skip to content

fix: add ref handling for useEventListener #2939

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Nov 23, 2024
Merged

fix: add ref handling for useEventListener #2939

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a188f4b
add refobject handling
Light2Dark Nov 23, 2024
d39d138
reduce variables
Light2Dark Nov 23, 2024
74a57fb
update types
Light2Dark Nov 23, 2024
ddcf824
add unit test for isRefObject
Light2Dark Nov 23, 2024
7c3c74b
pass in refObj instead of ref.current
Light2Dark Nov 23, 2024
4451bca
update comment
Light2Dark Nov 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion frontend/src/components/debugger/debugger-code.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@
}}
extensions={[
langs.shell(),
EditorView.updateListener.of((update) => {

Check warning on line 58 in frontend/src/components/debugger/debugger-code.tsx

View workflow job for this annotation

GitHub Actions / 🖥️ Lint, test, build frontend 

Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef)
if (update.docChanged) {
ref.current.view?.dispatch({
selection: {
Expand All @@ -79,7 +79,7 @@
const ref = React.useRef<HTMLDivElement>(null);

// Capture some events to prevent default behavior
useKeydownOnElement(ref.current, {
useKeydownOnElement(ref, {
ArrowUp: Functions.NOOP,
ArrowDown: Functions.NOOP,
});
Expand Down
2 changes: 1 addition & 1 deletion frontend/src/components/editor/Cell.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,7 +405,7 @@ const CellComponent = (
},
});

useKeydownOnElement(editing ? cellRef.current : null, {
useKeydownOnElement(editing ? cellRef : null, {
ArrowDown: () => {
moveToNextCell({ cellId, before: false, noCreate: true });
return true;
Expand Down
24 changes: 24 additions & 0 deletions frontend/src/hooks/__tests__/useEventListener.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/* Copyright 2024 Marimo. All rights reserved. */
import { expect, describe, it } from "vitest";
import { renderHook } from "@testing-library/react-hooks";
import { useRef } from "react";
import { isRefObject } from "../useEventListener";

describe("isRefObject", () => {
it("should return true for React ref objects", () => {
const { result } = renderHook(() => useRef(null));
expect(isRefObject(result)).toBe(true);
});

it("should return false for non-ref values", () => {
expect(isRefObject(null)).toBe(false);
expect(isRefObject(123)).toBe(false);
expect(isRefObject(document.createElement("div"))).toBe(false);
expect(isRefObject(document)).toBe(false);
expect(isRefObject(window)).toBe(false);
});

it("should return true for objects with 'current' property", () => {
expect(isRefObject({ current: document.createElement("div") })).toBe(true);
});
});
17 changes: 14 additions & 3 deletions frontend/src/hooks/useEventListener.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
/* Copyright 2024 Marimo. All rights reserved. */
import { useRef, useEffect } from "react";
import { useRef, useEffect, type RefObject } from "react";

type Target = Document | HTMLElement | Window | null;
type TargetRef = RefObject<Target>;

type EventMap<T extends Target> = T extends Document
? DocumentEventMap
: T extends HTMLElement
Expand All @@ -10,8 +12,14 @@ type EventMap<T extends Target> = T extends Document
? WindowEventMap
: never;

export function isRefObject<T>(
target: T | RefObject<T>,
): target is RefObject<T> {
return target !== null && typeof target === "object" && "current" in target;
}

export function useEventListener<T extends Target, K extends keyof EventMap<T>>(
target: T,
targetValue: T | TargetRef,
type: K & string,
listener: (ev: EventMap<T>[K]) => unknown,
options?: boolean | AddEventListenerOptions,
Expand All @@ -23,6 +31,9 @@ export function useEventListener<T extends Target, K extends keyof EventMap<T>>(
}, [listener]);

useEffect(() => {
// Get the actual target, whether it's from a ref or direct value
// We get ref.current inside the effect instead of during render because changes to ref.current will not trigger a re-render
const target = isRefObject(targetValue) ? targetValue.current : targetValue;
if (!target) {
return;
}
Expand All @@ -34,5 +45,5 @@ export function useEventListener<T extends Target, K extends keyof EventMap<T>>(
return () => {
target.removeEventListener(type, eventListener, options);
};
}, [type, target, options]);
}, [type, targetValue, options]);
Copy link
Contributor

@mscolnick mscolnick Nov 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are passing the RefObject, wont this not trigger re-renders when it changes?

Although if this fixes the underlying issues, I trust this is the right change.

Copy link
Contributor Author

@Light2Dark Light2Dark Nov 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mutating the ref.current value won't trigger re-renders. But the ref object itself does. I'm basing this because you would normally get a warning when using ref.current in useEffect

Mutable values like 'ref.current' aren't valid dependencies because mutating them doesn't re-render the component. (react-hooks/exhaustive-deps)

But I've searched online and see that a common solution is to use useCallback, but I think that would require a lot of code modification. Do you think I should look more in this direction?

update: we could just use const [element, setElement] = useState(cellRef); and pass in element to the hooks. This is way simpler and has the same effect & logic imo.

Let me revert if I don't find something better.

Copy link
Contributor

@mscolnick mscolnick Nov 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this fixes it, i am totally ok with it! i think this was a bug that was already there, but I added ReactCompiler which memoizes a lot such that subtle bugs like this could crop up.

also, you might have fixed this #2940 (thank you!)

Copy link
Contributor

@mscolnick mscolnick Nov 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking at this implementation, looks like you are right: https://usehooks-ts.com/react-hook/use-event-listener

and we should be passing in the Ref

}
6 changes: 3 additions & 3 deletions frontend/src/hooks/useHotkey.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/* Copyright 2024 Marimo. All rights reserved. */
import { useEffect } from "react";
import { type RefObject, useEffect } from "react";

import { parseShortcut } from "../core/hotkeys/shortcuts";
import { useEventListener } from "./useEventListener";
Expand Down Expand Up @@ -57,7 +57,7 @@ export function useHotkey(shortcut: HotkeyAction, callback: HotkeyHandler) {
* Registers a hotkey listener on a given element.
*/
export function useHotkeysOnElement<T extends HotkeyAction>(
element: HTMLElement | null,
element: HTMLElement | RefObject<HTMLElement> | null,
handlers: Record<T, HotkeyHandler>,
) {
const hotkeys = useAtomValue(hotkeysAtom);
Expand All @@ -81,7 +81,7 @@ export function useHotkeysOnElement<T extends HotkeyAction>(
* Registers a hotkey listener on a given element.
*/
export function useKeydownOnElement(
element: HTMLElement | null,
element: HTMLElement | RefObject<HTMLElement> | null,
handlers: Record<string, HotkeyHandler>,
) {
useEventListener(element, "keydown", (e) => {
Expand Down
Loading