nandorojo / dripsy

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

Missing intellisense for variants #147

Closed Jomik closed 2 years ago

Jomik commented 2 years ago

Hey, I tried following your guides to set up variants for my Text component, but it TypeScript refuses to acknowledge the keys under text in MyTheme.

I noticed that Text gets the prop type StyledProps<'text', never> & TextProps Manually updating never to DripsyVariant<'text'> solves the issue, and gives me correct suggestions. When looking at the source for createThemeComponent, then it seems like that is also the intention: https://github.com/nandorojo/dripsy/blob/798c6819c01ce4203e585739235b37b7f5b7efe9/packages/core/src/css/create-themed-component.tsx#L20 https://github.com/nandorojo/dripsy/blob/798c6819c01ce4203e585739235b37b7f5b7efe9/packages/core/src/css/types.ts#L287-L290 Has it been built wrong?

I have dumped my files for a repro into a gist: https://gist.github.com/Jomik/faa0f1ca6c7f46ba179c3bdfb60b1d0c

Versions:

{
    "dripsy": "^3.2.1",
    "typescript": "~4.4.4"
}
nandorojo commented 2 years ago

Yeah I think there’s an issue with intellisense for variants. That’s what you mean right?

Jomik commented 2 years ago

It simply seems like the Text component has the wrong type. The second Generic is StyledProps is never which results in it accepting no values for the variant prop. The type of the prop becomes undefined. So it results in a type error, not simply lack of type narrowing.

nandorojo commented 2 years ago

Yeah but can you confirm that what I said is the issue you’re dealing with? Like you’re trying to use the variant prop but not getting intellisense, right?

Jomik commented 2 years ago

The primary issue is that I am getting a type error because the type is not even string, it is undefined. As a result, I am obviously not getting intellisense either 😊

If I remove the variantFallbackType Property from my theme, then the type becomes string.

I updated the link for the gist, it was invalid for some reason. https://gist.github.com/Jomik/faa0f1ca6c7f46ba179c3bdfb60b1d0c#file-theme-ts-L18

nandorojo commented 2 years ago

What type error? Can you include all relevant info like that in this issue?

nandorojo commented 2 years ago

Why did you set variantFallbackType: undefined

nandorojo commented 2 years ago

I think that would cause this type error, you should leave that to string & {} (the default)

I’ll look into the intellisense separately

Jomik commented 2 years ago

I set it to undefined, to follow https://github.com/nandorojo/dripsy/blob/798c6819c01ce4203e585739235b37b7f5b7efe9/packages/core/src/declarations.ts#L203-L205 If that is unsupported, then I will just set it as you say 😊

nandorojo commented 2 years ago

Ah got it, I left this undocumented I think

nandorojo commented 2 years ago

I think I found the issue

nandorojo commented 2 years ago

@jomik can you try upgrading to canary and lmk if that solves it?

yarn add dripsy@canary

Works for me:

image

image

nandorojo commented 2 years ago

PR at #148

nandorojo commented 2 years ago

PS if you're using gradient too, upgrade that as well

yarn add dripsy@canary @dripsy/gradient@canary
Jomik commented 2 years ago

Using version 3.4.1-alpha.0. I have restarted VSCode. I currently have

  types: {
    reactNativeTypesOnly: true,
    onlyAllowThemeValues: 'always',
    strictVariants: true,
  },

The expected type of variant is undefined. image

nandorojo commented 2 years ago

Can I see your theme file

Jomik commented 2 years ago

Can I see your theme file

Yes! I have updated the gist: https://gist.github.com/Jomik/faa0f1ca6c7f46ba179c3bdfb60b1d0c#file-theme-ts

nandorojo commented 2 years ago

I’ll look when I’m at my computer at this thanks

nandorojo commented 2 years ago

Weird. works in the example app locally but not in my app either. will need to see why

Jomik commented 2 years ago

What happens locally if you explicitly set never as the second generic for StyledProps locally? Because that's what the released package has, after the compile step.

nandorojo commented 2 years ago

yeah, that just indicates that it isn't properly reading in the theme

nandorojo commented 2 years ago

Oh yeah I see what you mean...wtf. Okay so it's a build issue. I'm thinking because it's importing from ..?

This would explain why it works for me locally.

But why is it importing like that....

Screen Shot 2021-11-12 at 6 08 42 PM
nandorojo commented 2 years ago

I think I'm finding where this is facing issues.

nandorojo commented 2 years ago
Screen Shot 2021-11-12 at 6 28 38 PM

This is what the dripsy example sees...

nandorojo commented 2 years ago

omgggggggg i think (hope) I got it. fucking typescript haha. if this is it, i'll be quite happy haha.

Before:

export type StyledProps<ThemeKey extends keyof DripsyFinalTheme, Variant = DripsyVariant<ThemeKey>> = {
  as?: ComponentType<any>
  variant?: Variant
  themeKey?: ThemeKey
  sx?: SxProp
  variants?: Variant[]
}

After:

export type StyledProps<ThemeKey extends keyof DripsyFinalTheme> = {
  as?: ComponentType<any>
  variant?: DripsyVariant<ThemeKey>
  themeKey?: ThemeKey
  sx?: SxProp
  variants?: DripsyVariant<ThemeKey>[]
}

I think putting a value that gets generated later (DripsyVariant<ThemeKey>) as a generic makes it get evaluated upfront, thus resolving in never? 🧐

There's a lesson to not try to "memoize" TS generics, and instead compute them inline.

nandorojo commented 2 years ago

yeah that fixes it for me. fixed in 3.5.2. thanks for peaking in the built version.

nandorojo commented 2 years ago

3.5.3 should also fix this prop for components without a themeKey, such as View. Man, this was a tough one. all done though 🥳

nandorojo commented 2 years ago

From my app (not the example app):

Screen Shot 2021-11-12 at 7 05 06 PM Screen Shot 2021-11-12 at 7 05 28 PM
Jomik commented 2 years ago

That's amazing! 💪 Yeah. I'm guessing they try to resolve as much as possible to avoid having the cost for consumers, which sucks when you plan on having your users extend on a type. 😅

nandorojo commented 2 years ago

Yeah, definitely a unique use case. There isn't much content out there for this kind of thing haha. But I know for next time.

chakrihacker commented 2 years ago

I still see variant issue on Text Component from dripsy. using 3.5.3

nandorojo commented 2 years ago

Hm weird, it’s working for me. Could you try reproducing on CodeSandbox maybe?

Do you also have @dripsy/gradient installed? If so, make sure to update that too.