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

Issue with DragEventHandler<keyof HTMLElementTagNameMap> Type #407

Closed
in-ch opened this issue Sep 19, 2024 · 6 comments
Closed

Issue with DragEventHandler<keyof HTMLElementTagNameMap> Type #407

in-ch opened this issue Sep 19, 2024 · 6 comments

Comments

@in-ch
Copy link
Contributor

in-ch commented Sep 19, 2024

Hello. 😀

When using the Panel component with event handlers such as onDragLeave, onDragEnter, onDrop, and onDragOver typed as DragEventHandler<keyof HTMLElementTagNameMap>, a type error occurs when trying to use the contains method, which is a method of HTMLElement.

Here's an example where the issue occurs:

const handleDragLeave: DragEventHandler<keyof HTMLElementTagNameMap> = (
  e: DragEvent<keyof HTMLElementTagNameMap>,
): void => {
  if (e.currentTarget.contains(e.relatedTarget)) return;
  setIsDragActive(false);
};

스크린샷 2024-09-12 12 20 00

The issue is that the keyof HTMLElementTagNameMap type represents string keys for tag names, so it cannot infer the types of methods (e.g., contains) that the event object has.

Therefore, the DragEventHandler<keyof HTMLElementTagNameMap> type is limited in using methods of HTMLElement.

How about using a union type or something similar to make it compatible with both keyof HTMLElementTagNameMap and HTMLElement types?

@bvaughn
Copy link
Owner

bvaughn commented Sep 20, 2024

How about using a union type or something similar to make it compatible with both keyof HTMLElementTagNameMap and HTMLElement types?

I don't think this fixes the typing situation you describe (at least not when I just did a quick check locally). Want to share a TypeScript REPL or something?

@in-ch
Copy link
Contributor Author

in-ch commented Sep 21, 2024

Thank you for your response. 😀

I am sharing the CodeSandbox link for part of the code used in the project.

In the useDrag hook, I defined the handleDragLeave function using the type React.DragEvent<HTMLDivElement> in order to use the currentTarget value from the event object.

However, when I wrote the code this way, I encountered the following error in the onDragLeave function of the component:

Type '(e: React.DragEvent<HTMLDivElement>) => void' is not assignable to type 'DragEventHandler<keyof HTMLElementTagNameMap>'.
  Types of parameters 'e' and 'event' are incompatible.
    Type 'DragEvent<keyof HTMLElementTagNameMap>' is not assignable to type 'DragEvent<HTMLDivElement>'.
      Type 'string' is not assignable to type 'HTMLDivElement'.
        Type 'string' is not assignable to type 'HTMLDivElement'.typescript(2322)
index.d.ts(1559, 9): The expected type comes from property 'onDragLeave' which is declared here on type 'IntrinsicAttributes & Omit<HTMLAttributes<keyof HTMLElementTagNameMap>, "id" | "onResize"> & { className?: string | undefined; ... 11 more ...; tagName?: keyof HTMLElementTagNameMap | undefined; } & { ...; } & RefAttributes<...>'

@in-ch
Copy link
Contributor Author

in-ch commented Sep 21, 2024

Since the onDragLeave prop is defined as React.DragEventHandler<keyof HTMLElementTagNameMap>, TypeScript cannot correctly infer the currentTarget property in the event object.
To avoid TypeScript compilation errors in the project code, I was able to resolve it by using a union type like this:

const handleDragLeave: DragEventHandler<keyof HTMLElementTagNameMap> = (
  e: DragEvent<keyof HTMLElementTagNameMap>,
): void => {
  if (e.currentTarget.contains(e.relatedTarget)) return;
  setIsDragActive(false);
};

In my opinion, for greater flexibility in types, it would be better to define the onDragEvent, onDragLeave, onDragOver, and onDrop props in the Panel component as follows to increase type flexibility and accuracy:

React.DragEventHandler<keyof HTMLElementTagNameMap | HTMLElementTagNameMap>

This will prevent type conflicts and ensure compatibility when handling drag events.🙏

@bvaughn
Copy link
Owner

bvaughn commented Sep 21, 2024

React.DragEventHandler<keyof HTMLElementTagNameMap | HTMLElementTagNameMap>

I think you maybe meant...

React.DragEventHandler<keyof HTMLElementTagNameMap | HTMLElement >

But assuming I'm understanding you correctly...

import { createElement, HTMLAttributes } from "react";

type Props = HTMLAttributes<keyof HTMLElementTagNameMap | HTMLElement>;

function Component(props: Props) {
  return createElement("div");
}

const component = createElement(Component, {
  onDragLeave: event => {
    // @ts-expect-error This still errors
    event.currentTarget.contains()
  }
});

That doesn't work.

Property 'contains' does not exist on type 'EventTarget & (HTMLElement | keyof HTMLElementTagNameMap)'.
Property 'contains' does not exist on type 'EventTarget & "object"'.(2339)

This works (at least for your "contains" example) but would otherwise be a breaking change for this library, I think:

type Props = HTMLAttributes<HTMLElement>

So can you share some code that shows how this would work?

@in-ch
Copy link
Contributor Author

in-ch commented Sep 23, 2024

It definitely works well when done like this:

type Props = HTMLAttributes<HTMLElement>;

In the previous code, the type of currentTarget might have been a combination of string and HTMLElement.
This means that currentTarget could have a type of string (which is a key of HTMLElement), causing the type check for the contains method to fail.

HTMLElement is generally supported for all tags included in keyof HTMLElementTagNameMap. In other words, HTMLElement represents typical DOM elements, so it can be used with various elements like <div>, <span>, etc. However, if there are compatibility issues with the library, I will look for compatible types.

Thank you for your response. 😀

in-ch added a commit to in-ch/react-resizable-panels that referenced this issue Sep 27, 2024
in-ch added a commit to in-ch/react-resizable-panels that referenced this issue Sep 27, 2024
bvaughn pushed a commit that referenced this issue Sep 28, 2024
related to (#407)

Instead of fixing the `tagName` type to `keyof HTMLElementTagNameMap`,
it has been made flexible by using a generic type.
This allows TypeScript to better infer `event` object types like
handleDrop when a specific tagName is provided in the component.

```jsx
<Panel
    onDrop={(e: DragEvent<HTMLDivElement>) => {
      if (e.currentTarget.contains(e.relatedTarget)) return;
    }}
    className={styles.PanelRow}
    defaultSize={20}
    minSize={10}
    tagName="div"
  >
    <div className={styles.Centered}>left</div>
</Panel>

```
@bvaughn
Copy link
Owner

bvaughn commented Sep 28, 2024

Fixed via #414; fix has been released in react-resizable-panels@2.1.4


❤️ → ☕ givebrian.coffee

@bvaughn bvaughn closed this as completed Sep 28, 2024
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

2 participants