nandorojo / dripsy

🍷 Responsive, unstyled UI primitives for React Native + Web.
https://dripsy.xyz
MIT License
1.99k stars 77 forks source link

fix exported types #239

Closed jjenzz closed 1 year ago

jjenzz commented 1 year ago

Fixes issue mentioned here https://github.com/nandorojo/dripsy/pull/237#issuecomment-1247307276

The styled.d.ts definition was trying to manually construct the Props type and looks like it was getting it wrong somewhere. Exporting the type will ensure it uses import("./create-themed-component").Props correctly instead.

DIFF

export declare function styled<
  C extends ComponentType<any>,
  ThemeKey extends keyof DripsyFinalTheme = keyof DripsyFinalTheme
>(
  Component: C,
  {
    themeKey,
    defaultVariant,
  }?: Pick<ThemedOptions<any, ThemeKey>, 'themeKey' | 'defaultVariant'>
): <Extra>(
  defaultStyle?:
    | import('./types').Sx
    | ((props: Extra) => import('./types').Sx)
    | undefined
) => import('react').ForwardRefExoticComponent<
  import('react').PropsWithoutRef<
    import('react').PropsWithChildren<
-      C extends ComponentType<infer BaseProps>
-        ? Omit<
-            {
-              [P in Exclude<
-                keyof BaseProps,
-                'variant' | 'variants'
-              >]: BaseProps[P]
-            },
-            keyof import('./types').StyledProps<ThemeKey_1> | keyof Extra
-          > &
-            Extra &
-            import('./types').StyledProps<ThemeKey>
-        : never
+      import('./create-themed-component').Props<C, Extra, ThemeKey>
    >
  > &
    import('react').RefAttributes<import('react').ElementRef<C>>
 >
vercel[bot] commented 1 year ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
dripsy ❌ Failed (Inspect) Sep 15, 2022 at 2:22PM (UTC)
nandorojo commented 1 year ago

Hmm but this still doesn't allow the styled<{ myProp: boolean }>() syntax, right?

jjenzz commented 1 year ago

oh, i didn't realise that was an intended api 😬 my bad.

what is the expectation if someone does styled<{ test: 'foo' }>(View)<{ test: 'bar' }>(); ?

jjenzz commented 1 year ago

i'll have to take a look another time as it is super late here now, sorry.

unfortunately, the C generic would need to be optional for that API to work and i don't know if it is possible to infer C if a type is passed for Props. doing so will cause C to fall back to its default value (instead of inference): https://github.com/microsoft/TypeScript/issues/43119

// would infer correctly
const a = styled(Text)()

// inference would break because `C` would fallback to its default value (likely `ComponentType<any>`)
const b = styled<{ prop?: boolean }>(Text)();

my solution relies on being able to infer the Text component type here in order to get its ElementRef and props 😞

the best solution for now might be to revert my change. or would you consider deprecating the broken api?

jjenzz commented 1 year ago

@nandorojo funny what a bit of sleep can do.

this should all be working now assuming the following tests look okay to you? note that when using styled<Props>(Comp)() api you must include Comp props as part of the Props generic (matches behavior in v3.7.4).

export const ExtendedCompWithExtraProps = styled<
  React.ComponentProps<typeof KeyboardAvoidingView> & { custom?: boolean }
>(KeyboardAvoidingView)({})

/* @ts-expect-error behavior cannot be 'test' */
export const ExtendE = () => <ExtendedCompWithExtraProps behavior="test" />
/* should not error */
export const ExtendF = () => <ExtendedCompWithExtraProps behavior="padding" />
/* @ts-expect-error custom cannot be string */
export const ExtendG = () => <ExtendedCompWithExtraProps custom="test" />
/* should not error */
export const ExtendH = () => <ExtendedCompWithExtraProps custom={false} />
nandorojo commented 1 year ago

this behavior looks right! i'll take a look today

jjenzz commented 1 year ago

@nandorojo great, thanks ❤️

also, just a heads up that for some reason installing older versions of dripsy installs the latest versions of types so my project has errors everywhere that i cannot silence with a downgrade 🙈

not sure why older versions don't pull types specific to that version but something for you to look into separately perhaps?

nandorojo commented 1 year ago

hmm it could be something related to the monorepo setup perhaps...not sure. hopefully publishing the newer version will fix it all

nandorojo commented 1 year ago

if you also have gradient installed, be sure to fix it at the same version. i'm going to fix this in v4

jjenzz commented 1 year ago

@nandorojo i think it may have been something to do with local caching (pnpm maybe 🤷 ). i've completely removed a bunch of stuff and reinstalled 3.6.0 and have the correct types, so that's a relief!

nandorojo commented 1 year ago

just published an updated version (3.8.1) with these changes

nandorojo commented 1 year ago

Hey @jjenzz! I think one of these PRs caused a type regression for ScrollView and TextInput. I can't quite figure out why. My best guess is that they have some unique ref type, since both of these components have custom methods like scrollTo() and focus(). That cause might be a red herring, but it would make sense to me...

Here's the issue: https://github.com/nandorojo/dripsy/issues/260

Thank you (& happy holidays!)