kobaltedev / kobalte

A UI toolkit for building accessible web apps and design systems with SolidJS.
https://kobalte.dev
MIT License
1.2k stars 61 forks source link

Missing type in PolymorphicProps type #78

Closed p7gg closed 1 year ago

p7gg commented 1 year ago

Describe the bug Current PolymorphicProps type is missing a type in the as prop causing an error when extending the component,

export type PolymorphicProps<Type extends As = As, Props = {}> = OverrideProps<
  ComponentProps<Type>,
  Props & { as?: Type }
>;

should be:

export type PolymorphicProps<Type extends As = As, Props = {}> = OverrideProps<
  ComponentProps<Type>,
  Props & { as?: Type | As }
>;

To Reproduce https://codesandbox.io/s/typescript-playground-export-forked-50p1o0?file=/index.tsx

  1. Go to 'index.tsx'
  2. Scroll down to 'App Component'
  3. See error

In this codesandbox I provided a fix which is in the fixedImpl.ts file, you can easily toggle between the imported files to see each implementation.

Expected behavior Should accept any ValidComponent passed to as prop

Screenshots image

Additional context This is actually the first issue I've opened, so, please, let me know if I can improve anything. I know its a small fix but I would be super down to do a PR for this, but that would be a first for me also and I think I'd need some guidance on that.

fabien-ml commented 1 year ago

Hi, thanks for reporting the issue.

Since the code is short I can add it myself.

However, i've noticed that your fix don't work in this case:

const Button = (props: ButtonProps) => {
  return <KobalteButton {...props} />;
};

// A button with additional custom props
const IconButton = (props: ButtonProps & { icon: JSX.Element }) => {
  return <KobalteButton {...props} />;
};

const App = () => {
  return (
    <Button 
      as={IconButton} 
      icon="icon" {/* error here: prop not recognized */}
    >
      Button
    </Button>
  );
};

The only solution that i've found is to wrap Button itself with createPolymorphicComponent which create an ugly type definition.

If you have any idea feel free to make a PR and thanks for your time.

p7gg commented 1 year ago

Hi, thanks for reporting the issue.

Since the code is short I can add it myself.

However, i've noticed that your fix don't work in this case:

const Button = (props: ButtonProps) => {
  return <KobalteButton {...props} />;
};

// A button with additional custom props
const IconButton = (props: ButtonProps & { icon: JSX.Element }) => {
  return <KobalteButton {...props} />;
};

const App = () => {
  return (
    <Button 
      as={IconButton} 
      icon="icon" {/* error here: prop not recognized */}
    >
      Button
    </Button>
  );
};

The only solution that i've found is to wrap Button itself with createPolymorphicComponent which create an ugly type definition.

If you have any idea feel free to make a PR and thanks for your time.

The only thing I can come up with is to wrap OverrideProps in one of these, with one small change: https://github.com/sindresorhus/type-fest/blob/main/source/simplify.d.ts

Should be:

export type Simplify<T> = T extends any ? { [KeyType in keyof T]: T[KeyType] } : T;

Then the type output should be flattened out, which I prefer tbh, even over the simplest type name showing up as hint.

ceopaludetto commented 1 year ago

I was able to use like so:

import type { PolymorphicProps, As } from "@kobalte/utils";
import type { JSX } from "solid-js";

import { Button as KButton } from "@kobalte/core";
import { splitProps } from "solid-js";

type ButtonOwnProps = KButton.ButtonRootOptions & {
  children?: JSX.Element
  class?: string // your custom props
}

export type ButtonProps<T extends As = "button"> = PolymorphicProps<T, ButtonOwnProps>;

export function Button<T extends As = "button">(props: ButtonProps<T>): JSX.Element {
  const [local, rest] = splitProps(props, ["class", "children"]);

  return (
    <KButton.Root class={local.class} {...rest}>
      {local.children}
    </KButton.Root>
  );
}

And the usage:

function App() {
  return (
    <Button as="a" href="http://it.works.com">Click me!</Button>
  )
}
p7gg commented 1 year ago

Oh yeah, its a nice workaround. I'll just wait for the fix since I'm still in dev