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

Missing type in PolymorphicProps type #78

Closed
p7gg opened this issue Jan 24, 2023 · 4 comments · Fixed by #87
Closed

Missing type in PolymorphicProps type #78

p7gg opened this issue Jan 24, 2023 · 4 comments · Fixed by #87

Comments

@p7gg
Copy link

p7gg commented Jan 24, 2023

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
Copy link
Collaborator

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.

fabien-ml added a commit that referenced this issue Jan 24, 2023
@p7gg
Copy link
Author

p7gg commented Jan 24, 2023

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
Copy link

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
Copy link
Author

p7gg commented Jan 25, 2023

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

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

Successfully merging a pull request may close this issue.

3 participants