tailwindlabs / heroicons

A set of free MIT-licensed high-quality SVG icons for UI development.
https://heroicons.com
MIT License
21.07k stars 1.27k forks source link

React Package has Breaking Change in 1.0.6 due to `forwardRef` #639

Closed pantajoe closed 1 year ago

pantajoe commented 2 years ago

Hello there! First, thanks for this amazing icon library! ❤️

I use dependabot in my project to update the dependencies on a regular basis. Thus, without suspecting anything, I reviewed the PR that updates @heroicons/react from 1.0.5 to 1.0.6 together with its changes.

However, upgrading this dependency broke my app 😅 I relied on the types of the React package to embed heroicons and other "componentified" SVG icons into my custom Button components like this:

https://github.com/tailwindlabs/heroicons/blob/71b15b9e72e0211c5dbb28fb75f280deedaec28d/scripts/build.js#L96

// props
export type BaseButtonType = 'button' | 'a';

export type BaseButtonProps<T extends BaseButtonType> = {
  as?: T;
  // ...
  icon?: (props: ComponentProps<'svg'>) => JSX.Element;
} & ComponentProps<T>;

type BaseButtonPropsWithRef<T extends BaseButtonType> = {
  innerRef?: ForwardedRef<T extends 'button' ? HTMLButtonElement : HTMLAnchorElement>;
} & BaseButtonProps<T>;

const BaseButton = <T extends BaseButtonType>({ as, children, icon, innerRef, ...props }: BaseButtonPropsWithRef<T>) => {
  return createElement(
    as ?? 'button',
    {
      ...props,
      ref: innerRef,
    },
    // the following line breaks the app
    icon?.({ className: leadingIconCssClasses, 'aria-hidden': 'true' }),
    children,
  );
};

In #614, you introduced accessing the ref on the SVGs. However, this results in those components not being callable as a function as they are now exotic components, i.e., they are now objects of the following structure:

import { PlusIcon } from '@heroicons/react/solid';

console.log(PlusIcon);
// => Object {
//   $$typeof: Symbol(react.forward_ref),
//   render: (props, ref) => { ... },
// }

And this behavior is a breaking change for me which React also suggests. However, I'm aware that this might be a niche problem and maybe not relevant for most users of this library.

I now reverted the upgrade until I come up with something that allows me to upgrade to the newest version while maintaining my api's flexibility. But it would be great if you could update the typings of this package to ForwardRefExoticComponents, so that TypeScript does not compile when relying on the previously used types of this package like me.

RobinMalfait commented 1 year ago

Hey! Thank you for your bug report! Much appreciated! 🙏

We just released v2 and I don't think we will be making changes to the v1 version anymore. However, I'm curious what you mean by:

However, this results in those components not being callable as a function as they are now exotic components

A "functional" component like:

function MyComponent() {
  return <div />
}

Should never be called like MyComponent(), but should always be used with JSX like <MyComponent /> or React.createElement(MyComponent).

Were you calling the icons as actual functions like ArchiveIcon()?

pantajoe commented 1 year ago

Hi! Thanks for your answer. You are completely right with your observation. When I submitted this issue, I was quite new to React 😅

Sorry for the rather dumb example. I wanted to assign the heroicons as a prop to buttons, i.e.,

<Button {...props} icon={ArchiveIcon} />

And I thought wrapping the entire button in createElement and calling the icon as a function from within the Button would be fine 😅

Simply using JSX notation solved everything. Also forgot to update and close this issue.

Thanks and sorry again for this "bug report"!