Recursively creating themed component doesn't work about dripsy HOT 17 CLOSED

I've tried to make some work-arounds but still haven't been able to debug this.

from dripsy.

export function createThemedComponent<P>(
	Component: ComponentType<P>,
	options: ThemedOptions = {}
): React.FC<P & Props<P>> {

...

// @ts-ignore
return React.memo(WrappedComponent)

This might be a decent workaround if the types are the only issue.

from dripsy.

I'll test that out.

It actually is more than a TS error, for some reason the actual renders don't work properly.

from dripsy.

Okay I think it actually is just a TS error for createThemedComponent, and that it's actually breaking for styled. I need to keep testing to be sure. If it is, @cmaycumber's work-around should do it:

export function createThemedComponent<P>(
	Component: ComponentType<P>,
	options: ThemedOptions = {}
): React.FC<P & Props<P>> {

...

// @ts-ignore
return React.memo(WrappedComponent)

In any case, I should probably re-write styled, either to 1) match createThemedComponent from scratch, or 2) to simply return createThemedComponent. Option 1 would allow us to pass a pure function to determine styles, option 2 would only allow a plain sx object.

The simplest rewrite would look like this:

export function styled<P>(
  Component: ComponentType<P>,
  {
    themeKey,
    defaultVariant,
  }: { themeKey?: string; defaultVariant?: string } = {}
) {
  return (sx: Required<SxProps>['sx']) => {
    return createThemedComponent(Component, {
      defaultStyle: sx,
      themeKey,
      defaultVariant,
    })
  }
}

Which would allow this usage:

import { View, styled } from 'dripsy'

// plain objects only
const Box = styled(View)({
  pb: 2
})

<Box sx={{ pb: 3 }} />

...but it would not allow this usage:

import { View, styled } from 'dripsy'

// pure function to determine styles
const Box = styled<{ pb: number }>(View)(props => ({
  pb: props.pb
}))

<Box pb={3} />

The passing around of props and refs gets pretty messy in React with this many HOCs. It seems like the current (and admittedly messy) implementation of styled is breaking somewhere along the way of recursively forwarding props and refs (or so I would guess).

from dripsy.

Another temporary workaround building off my previous one that fixes @expo/html-elements would add an additional optional child prop:

export function createThemedComponent<P>(
	Component: ComponentType<P>,
	options: ThemedOptions = {}
): React.FC<P & Props<P> & { children?: React.ReactNode }> {

...

// @ts-ignore
return React.memo(WrappedComponent)

Another thing to note React.FC can be replaced with ComponentType which may be more accurate because it shows a React.ForwardRefExoticComponent as the return type.

from dripsy.

Got it, I'll try this out. ComponentType probably makes more sense. I see that @expo/html-elements is using React.PropsWithChildren, I haven't used that type before.

I'm also working on figuring out a way to get React.memo to work without using @ts-ignore, such that it forwards the correct details. Something like this might work to forward the props generic:

const typedMemo: <T>(c: T) => T = React.memo
return typedMemo(WrappedComponent)

But I'm not sure if it works with forwarding refs too yet.

from dripsy.

For whatever reason, I cannot get the TS suggestions for SxProps['sx'] type to work when it's a return type of a function.

// types.ts
export type ThemedOptions<T> = {
  defaultStyle?:
    | SxProps['sx'] // this works fine with intellisense 
    | ((props: T) => SxProps['sx']) // no intellisense here...
  themeKey?: string
  defaultVariant?: string
}

I've also tried using SxStyleProp and ThemeUIObject, but no luck yet. When I use my own custom type, it works fine.

Here's the updated createThemedComponent file:

/* eslint-disable @typescript-eslint/ban-ts-ignore */
import React, { ComponentType, ComponentProps, useMemo } from 'react'
import { ThemedOptions, StyledProps } from './types'
import { useThemeUI } from '@theme-ui/core'
import { useBreakpointIndex, mapPropsToStyledComponent } from '.'
import { SSRComponent } from './ssr-component'
import { Platform } from 'react-native'

type Props<P> = Omit<StyledProps<P>, 'theme' | 'breakpoint'>

export function createThemedComponent<P, T>(
  Component: ComponentType<P>,
  { defaultStyle: baseStyle, ...options }: ThemedOptions<T> = {}
) {
  // without styled-components...
  const WrappedComponent = React.forwardRef<
    typeof Component,
    Props<P> & ComponentProps<typeof Component> & T
  >(function Wrapped(prop, ref) {
    const {
      sx,
      as: SuperComponent,
      variant,
      style,
      webContainerSx,
      themeKey = options.themeKey,
      ...props
    } = prop
    const defaultStyle =
      typeof baseStyle === 'function' ? baseStyle(prop) : baseStyle

    const { theme } = useThemeUI()
    const breakpoint = useBreakpointIndex({
      __shouldDisableListenerOnWeb: true,
    })
    // const ssr = useIsSSR()
    // change/remove this later maybe
    const ssr = Platform.OS === 'web'

    const { responsiveSSRStyles, ...styles } = useMemo(
      () =>
        mapPropsToStyledComponent(
          {
            theme,
            breakpoint: Platform.OS === 'web' && ssr ? undefined : breakpoint,
            variant,
            sx,
            style,
          },
          {
            ...options,
            themeKey,
            defaultStyle,
          }
        )(),
      [breakpoint, defaultStyle, ssr, style, sx, theme, themeKey, variant]
    )

    const TheComponent = SuperComponent || Component

    if (Platform.OS === 'web' && ssr && !!responsiveSSRStyles?.length) {
      return (
        <SSRComponent
          {...props}
          Component={TheComponent as React.ComponentType<unknown>}
          responsiveStyles={responsiveSSRStyles}
          style={styles}
          ref={ref}
          containerStyles={
            webContainerSx as ComponentProps<
              typeof SSRComponent
            >['containerStyles']
          }
        />
      )
    }

    return (
      <TheComponent {...((props as unknown) as P)} ref={ref} style={styles} />
    )
  })

  WrappedComponent.displayName = `Themed.${Component.displayName ??
    'NoNameComponent'}`

  return React.memo(WrappedComponent)
}

Once I get this fixed I'll clean up the code and remove old stuff that went unused. The naming is a bit confusing in a number of places, too.

@cmaycumber I haven't updated to put your explicit return type changes yet, I can add that once I figure this out.

from dripsy.

There are also issues with merging the default style and a new sx prop sometimes.

from dripsy.

I'll try to take a look at this as well.

from dripsy.

Adding this: React.FC<P & Props<P>> as an explicit return type on the HOC seems to remove the TS error from the client and shifts it to the React.memo(WrappedComponent).

Not sure how important this is yet but I thought it was an interesting observation.

from dripsy.

Does the styled component currently support passing props?

I can think off the top of my head plenty of situations where passing a prop might be needed for determining the style even outside of the scope of theme-related styles.

from dripsy.

That usage is currently commented out. I got a lot of weird errors/edge cases importing the SxProps type from theme-ui when originally trying to make it, so I left it for later. SxProps seems like a pretty unusual type.

I came up with this:

import React, { ComponentProps, ComponentType } from 'react'
import { SxProps } from '@theme-ui/core'
import { createThemedComponent } from './create-themed-component'
import { StyledProps } from './types'

type Props<P> = Omit<StyledProps<P>, 'theme' | 'breakpoint'>
export function styled<P>(
  Component: ComponentType<P>,
  { themeKey, defaultVariant }: { themeKey?: string; defaultVariant?: string } = {}
) {
  return (sx?: ((props: P) => Required<SxProps>['sx']) | Partial<Required<Required<SxProps>['sx']>>) => {
    const Styled = React.forwardRef<typeof Component, Props<P> & ComponentProps<typeof Component>>(function Styled(
      props,
      ref
    ) {
      const Themed = createThemedComponent(Component, {
        defaultStyle: typeof sx === 'function' ? sx(props) : sx,
        themeKey,
        defaultVariant
      })
      // @ts-ignore
      return <Themed {...(props as unknown) as P} ref={ref} />
    })
    Styled.displayName = `DripsyStyled.${Component.displayName}`

    return Styled
  }
}

But that actually doesn't work perfectly either, because Component then needs to have props P declared in its prop types, when in reality we don't need it to if it's just for the sake of styling.

(For whatever reason, I wasn't getting typed suggestions until I added the many Partial and Required type helpers.)

The problems are 1) why doesn't styled allow recursive creation, and 2) how do we allow a pure function as a style argument with type suggestions.

from dripsy.

Recursively calling forwardRef looks like it's causing the TS errors from createThemedComponent.

All the @expo/html-elements also forward ref and those are the only primitive components we have that show TS errors when you use them.

from dripsy.

Not sure where the best place to discuss styled is, but as I mentioned on #40, I had the idea to move the pure function logic into createThemedComponent.

It seems like a refactoring like this would work (oversimplified for demonstration):

function createThemedComponent<P, T>(Component: React.ComponentType<P>, defaultStyle: SxProp | ((t: T) => SxProp)) {
  return function FinalComponent(props: P & T) {
    const style = typeof defaultStyle === "function" ? defaultStyle(props) : defaultStyle;

    return <Component {...props} style={style} />;
  };
}

I made a code sandbox to demonstrate this.

You can use it like so:

/**
 * Here's a random test component
 */
const H1 = ({ i }: { i: boolean }) => <h1>{`${i}`}</h1>;

const Created = createThemedComponent(H1, { padding: 1 });
const EnhancedCreated = createThemedComponent(H1, (props: { p: number }) => ({
  padding: props.p
}));

This gives us all the proper types:

from dripsy.

The TS errors seen in the H1 and other elements is fixed in 1.2.1. I used @cmaycumber's suggestion of explicitly defining the component's return type.

Recursively using styled or createThemedComponent still seems like it isn't working. I'll be doing some extensive testing of this.

from dripsy.

I just randomly had an idea. Could solving this issue be as simple as not theming the style prop?

from dripsy.

I think I fixed this in v2.1.0.

from dripsy.