bem / bem-react

A set of tools for developing user interfaces using the BEM methodology in React
http://bem.github.io/bem-react
Other
439 stars 64 forks source link

withBemMod is no more compatible with compose #594

Closed fiberthrone closed 3 years ago

fiberthrone commented 3 years ago

It turns out that withBemMod typings I've introduced in #586 (and hotfixed in #588) don't work quite as expected.

With the new typings withBemMod doesn't work properly with compose anymore. compose(a, b, ...) results in Composition<any> instead of Composition<PropsA, PropsB, ...>:

interface IComponentProps {
  theme?: 'normal';
  view?: 'default';
}

const withTheme = withBemMod<IComponentProps>('Component', { theme: 'normal' });
const withView = withBemMod<IComponentProps>('Component', { view: 'default' });

// In 3.0.1 the type of withMods would be Composition<IComponentProps, IComponentProps>
// but in 3.0.2 it is Composition<any>
const withMods = compose(withTheme, withView);

As I figured out, Composition<any> is just a fallback after a list of proper overloads:

export type HOC<T> = (WrappedComponent: ComponentType) => ComponentType<T>
export function compose<T1>(fn1: HOC<T1>): Composition<T1>
export function compose<T1, T2>(fn1: HOC<T1>, fn2: HOC<T2>): Composition<T1 & T2>
// a couple more similar overloads up to 8 arguments
export function compose(...fns: Array<HOC<any>>): Composition<any>

The reason for the issue is that the new typings are a little too honest in their way of handling refs.

I've made three overloads of withBemMod: one for class components, one for ref forwarding function-ish components, and one for regular function components:

interface IWithBemMod {
  <K extends IClassNameProps, Q extends React.ComponentClass<T & K>>(
    WrappedComponent: Q,
  ): React.ForwardRefExoticComponent<
    React.ComponentPropsWithoutRef<Q> & { ref?: React.Ref<InstanceType<Q>> }
  >

  <K extends IClassNameProps>(
    WrappedComponent: React.ForwardRefExoticComponent<T & K & { ref?: React.Ref<any> }>,
  ): React.ForwardRefExoticComponent<T & K & { ref?: React.Ref<any> }>

  <K extends IClassNameProps>(
    Component: React.FunctionComponent<T & K>,
  ): React.ForwardRefExoticComponent<T & K>
  // ...
}

Every one of these returns a component with a ref prop, except for the last one, – it seemed like a good idea to me, since React does not support refs for function components. But because of that withBemMod doesn't return a HOC<T> anymore, since it's not determined whether or not T has a ref property. So only the Composition<any> overload is applicable for the new withBemMod typings.

One way to fix this is to make typings more loose and make a withBemMod-modified component to always have a ref prop. But it doesn't look like a safe solution to me – it might make the library users to pass refs to function components by mistake without a typechecker warning.

So I've come up with a fix with more strict typings: #593. Basically, it reverts typings to 3.0.1, but keeps the ref forwarding. It might seem hacky, but at least it doesn't break anything.