nandorojo / dripsy

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

Basic usage of variants broken? #283

Closed jeffscottward closed 1 year ago

jeffscottward commented 1 year ago

Following the most basic example from the docs site.

Screenshot 2023-04-25 at 10 05 58 AM Screenshot 2023-04-25 at 10 06 38 AM

Nets this error:

Type 'string' is not assignable to type 'undefined'.ts(2322)
types.d.ts(85, 5): The expected type comes from property 'variant' which is declared here on type 'IntrinsicAttributes & (StyledProps<"text"> & ((TextProps & RefAttributes<Component<TextProps, any, any>>) | (TextProps & { ...; })))'
(property) variant: string

I am using the suggested mono repo with all the original configs unchanged.

Am I missing something?

nandorojo commented 1 year ago

can you upgrade dripsy and TS, as shown in the dripsy release notes?

jeffscottward commented 1 year ago

Ahh for the 4.0.0. release. https://github.com/nandorojo/dripsy/releases/tag/v4.0.0

Should it be within Next or at the Root?

I ask because there is a top level "typescript": "^4.7.4"

in that root package.json

jeffscottward commented 1 year ago

I attempted to bootstrap an enitrely new solito with-expo-router and paste in my files and it did not work. hmm

jeffscottward commented 1 year ago

Removing @dripsy/core as instructed results in errors

Failed to compile
../../node_modules/@dripsy/core/build/components/ScrollView.js
Module parse failed: Unexpected token (10:12)
You may need an appropriate loader to handle this file type, currently no loaders are configured to process this file. See https://webpack.js.org/concepts#loaders
|     const containerSx = props.contentContainerSx && sx(props.contentContainerSx);
|     const indicatorSx = props.indicatorSx && sx(props.indicatorSx);
>     return (<DripsyScrollView {...props} contentContainerStyle={props.contentContainerStyle && containerSx
|             ? [props.contentContainerStyle, containerSx]
|             : containerSx || props.contentContainerStyle} indicatorStyle={props.indicatorStyle && indicatorSx
jeffscottward commented 1 year ago

Putting it back in allows it to compile but still have the original error from above and also noticing it tripping on onClick, that probably was there before and I didn't notice.

nandorojo commented 1 year ago

If you're getting that error, you haven't upgraded properly. TypeScript should be set in the root, and you can use an override with resolutions to be safe. Dripsy should be the latest version.

nandorojo commented 1 year ago

Be sure to run yarn from the root and restart the dev server.

jeffscottward commented 1 year ago

If you're getting that error, you haven't upgraded properly. TypeScript should be set in the root, and you can use an override with resolutions to be safe. Dripsy should be the latest version.

Confirmed its not on my end. On a totally new npx create-solito-app@latest -t with-expo-router <View onClick and <Text variant are both showing typescript errors.

And then doing as suggested in the docs. No change after the yarn commands. No change after the vscode (with restart). And then it crashes when removing @dripsy/core

Screenshot 2023-04-25 at 1 25 29 PM Screenshot 2023-04-25 at 1 25 50 PM

Code is here https://github.com/jeffscottward/triplecheck

I manually made sure of Typescript being 5.0.4 too

jeffscottward commented 1 year ago

Be sure to run yarn from the root and restart the dev server.

did that too.

nandorojo commented 1 year ago

use yarn why dripsy and yarn why @dripsy/core to make sure you installed properly

jeffscottward commented 1 year ago

looks fine?

└─ dripsy@npm:3.10.0
   └─ @dripsy/core@npm:3.10.0 (via npm:^3.10.0)
jeffscottward commented 1 year ago

Wait why is it stuck on 3.1 UPDATED: Still TS errors

onClick no longer erroring but still the variant error

latest is on https://github.com/jeffscottward/triplecheck

yarn why dripsy

└─ solito-expo-router@workspace:.
   └─ dripsy@npm:4.1.0 (via npm:^4.1.0)
Screenshot 2023-04-25 at 1 45 39 PM
nandorojo commented 1 year ago

That repo is private

jeffscottward commented 1 year ago

That repo is private

Oo sorry. Changed. Also after restarting my typescript server, everything just went haywire.

All kinds of errors like Module '../layout/Header' was resolved to '/Users/jeffscottward/Documents/GitHub/vertex-xr/vertex.xyz/packages/app/features/layout/Header.tsx', but '--jsx' is not set.ts(6142)

Manually selecting the typescipt version to be 5+ does nothing.

nandorojo commented 1 year ago

maybe try restarting VSCode or something

jeffscottward commented 1 year ago

Did that. I’ll have to come back to all this later. You have my best stab at the code in the repo. Let me know if you find anything.

On Tue, Apr 25, 2023 at 2:09 PM Fernando Rojo @.***> wrote:

maybe try restarting VSCode or something

— Reply to this email directly, view it on GitHub https://github.com/nandorojo/dripsy/issues/283#issuecomment-1522205603, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJUO7X3EF2MWJTLSSVN74DXDAHL7ANCNFSM6AAAAAAXLCDNNU . You are receiving this because you authored the thread.Message ID: @.***>

-- Thanks, Jeff Ward Front-end Developer Tel: 516-551-8624 @. @.>* @.**** https://twitter.com/#!/jeffscottward

jeffscottward commented 1 year ago

The typescript stuff was a missing Expo folder in Node Modules at the top level. Idk how it happened. Variant issues is still true.

jeffscottward commented 1 year ago

I have confirmed its broken both in my actual project and the sample repo I posted for you. Best I can track it to is the changes you made here. https://github.com/nandorojo/dripsy/commit/0ab658a597e75bd45a9c48847de1492e7fed0ea1

It seems when the code used to use MaybeVariantsFromThemeKey https://github.com/nandorojo/dripsy/blob/e8272cd46261a34554c16fd370a536c5514e1312/packages/dripsy/src/core/types-v2/sx.ts#L350 that it was extending a string

jeffscottward commented 1 year ago

Have you verified this yet? @nandorojo

nandorojo commented 1 year ago

are you using numbers as keys?

jeffscottward commented 1 year ago

are you using numbers as keys?

nope. It's just the exact example from the start

https://github.com/nandorojo/dripsy/issues/283

https://github.com/jeffscottward/triplecheck/blob/80823fbc7c929cda42e5b81481569cb9516172ea/packages/app/features/home/screen.tsx#L19

https://github.com/jeffscottward/triplecheck/blob/80823fbc7c929cda42e5b81481569cb9516172ea/packages/app/theme.ts#L128

jeffscottward commented 1 year ago

Have you checked the triplecheck repo I made?

If you're unwilling to help, do you have a fully working with-expo-router project example I can diff against? triplecheck was my attempt to have a completely clean base and just that issue showcased.

jeffscottward commented 1 year ago

Bump. I hate to keep harping on this, but variants just do not work at all with this error. I need to resolve it.

nandorojo commented 1 year ago

Please keep in mind that this is open source and you could investigate yourself and PR. This isn't my job, I'm helping for free, and you aren't sponsoring me or the project. I'll look into it today and address it, but please be mindful of that before pinging.

Second, your reproduction you shared doesn't seem to follow what you want. The theme is this:

const theme = makeTheme({
  // https://www.dripsy.xyz/usage/theming/create
  text: {
    p: {
      fontSize: 16,
    },
  },
})

Where is the thick variant there? Also, there is no type declaration merging set up. I added both of these and it works right away, just as I mentioned in the docs.

Screenshot 2023-05-15 at 10 42 20 AM Screenshot 2023-05-15 at 10 42 36 AM

Everything works exactly as expected for me in your reproduction after I followed the docs for TypeScript, so I'm going to close this for now.

nandorojo commented 1 year ago

PS, see in this file, you need to add the type declaration merging for it to work: https://github.com/jeffscottward/triplecheck/blob/80823fbc7c929cda42e5b81481569cb9516172ea/packages/app/theme.ts

type Theme = typeof theme

declare module 'dripsy' {
  interface DripsyCustomTheme extends Theme {}
}
nandorojo commented 1 year ago
Screenshot 2023-05-15 at 10 49 01 AM

Works fine after doing that.

nandorojo commented 1 year ago

Would be useful to add this declaration merging to the Solito monorepo for future users to avoid this. I'd accept a PR on the Solito repo for that, or I'll try to get to it soon.

nandorojo commented 1 year ago

Also for future reference: if your example in the original post had the full theme file, I would have been able to find it more easily. The screenshots weren't enough to find it, full code samples are usually easier to debug. Thanks.

nandorojo commented 1 year ago

Also, apologies if that came off as harsh. Just wanted to clarify.

jeffscottward commented 1 year ago

Before I begin:

1) There is no link to sponsor you for this work. If you would like to provide one, I would strongly consider donating per each issue I might have or PR to review.

2) I did what digging I was capable of to point out where I think the issue might be in the Dripsy commits. I'm not a typescript expert and have limited capabilities to repair such an issue.

3) I did ask for a working example repo of which you wouldn't have had to do anything. I suggest creating one for someone to test and compare against so you don’t have to do what you just generously did for me above.

All that said, I appreciate that you poked at it very much.

I will do the links to code as opposed to screenshots from now on. In my experience, nothing beats “seeing” it, perhaps I default to that too much dealing with designers.

I’m glad the type merging was highlighted, definitely easy to overlook rushing into Dripsy.

I think this likely my only issue for a while given this was all part of a big upgrade. I’m supremely grateful for this tool and will have a huge impact for me. I’m literally building an entire cutting edge business on top of this, Solito and Moti.

Thanks 🙏

nandorojo commented 1 year ago

I will do the links to code as opposed to screenshots from now on. In my experience, nothing beats “seeing” it, perhaps I default to that too much dealing with designers.

I didn't mean links to code, rather using an inline code block here. But I agree it's hard to know what to share sometimes. And I appreciate your putting together the repro.

From my perspective: I'm putting out all this stuff for everyone to use while trying to build a startup. I could just keep it internally, but I want to help. I'm getting dozens of notifications a day across all of the projects, so I need try to keep them reasonably under control.

I understand that it feels like you did the most you could here to get info across for me to address the issue, so I appreciate that.

There is no link to sponsor you for this work. If you would like to provide one, I would strongly consider donating per each issue I might have or PR to review.

I didn't consider adding this directly on the repo, I figured GitHub just displayed the link to sponsor the profile. Good to know – and thank you!

You can find the link here: https://github.com/sponsors/nandorojo

I did ask for a working example repo of which you wouldn't have had to do anything. I suggest creating one for someone to test and compare against so you don’t have to do what you just generously did for me above.

Yeah, that's a good idea. I should update the Solito example to have these types.

I think this likely my only issue for a while given this was all part of a big upgrade. I’m supremely grateful for this tool and will have a huge impact for me. I’m literally building an entire cutting edge business on top of this, Solito and Moti.

Glad it's useful, and wishing you luck as you build your product.