Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TypeScript #49

Open
LandonSchropp opened this issue Jan 1, 2024 · 10 comments
Open

TypeScript #49

LandonSchropp opened this issue Jan 1, 2024 · 10 comments

Comments

@LandonSchropp
Copy link

Thank you for this plugin!

I was wondering, have you considered converting the plugin to TypeScript or adding types to this repo to support Tailwind's TypeScript configuration?

Screenshot 2024-01-01 at 10 50 30 AM

@crswll
Copy link
Owner

crswll commented Jan 3, 2024

No problem at all! I have considered adding types, even started a couple times but couldn't get it to be as intuitive as I want. I feel like this is pretty close but I don't really like having to do Swapper<typeof baseTheme>. Will take another crack at it soon and if you have any suggestions let me know.

@LandonSchropp
Copy link
Author

LandonSchropp commented Jan 4, 2024

I'm happy to jump in as well if you'd like. 🙂

Given the size of the repo, it might be easier to rewrite the source itself in TypeScript. Then the compiler generates the types for you and you don't have the extra maintenance burden of keeping the types in sync with the code.

I understand if you don't want to go down that road though.

@LandonSchropp
Copy link
Author

LandonSchropp commented Jan 4, 2024

I played with your config a bit and here's what I came up with. I wouldn't call myself a TypeScript expert though, and I'm not deeply familiar with the options in this repo, so I'd take that with a grain of salt.

@LandonSchropp
Copy link
Author

LandonSchropp commented Jan 4, 2024

In terms of preventing extra properties, I dug a bit and I don't think that's a thing TypeScript supports currently (GitHub issue). It seems like it could be possible using some mangled type magic, but it doesn't seem worth it to me.

@crswll
Copy link
Owner

crswll commented Jan 4, 2024

Yeah, it probably wouldn't hurt to write it in TypeScript. I mostly didn't because I didn't want to add a build step. It's likely worth it, though.

Your types are close but I think an important thing to make sure of is that the child themes can't declare theme values that aren't in the base theme which mine does but I don't think well. I'll start playing around with converting to TypeScript and maybe make some breaking changes I've wanted to.

@LandonSchropp
Copy link
Author

Sounds good. 👍

the child themes can't declare theme values that aren't in the base theme

I don't think I quite understand this req. Maybe you could give a quick example. No worries if you just want to tackle it yourself as well—I don't mean to backseat drive. 🙂

@crswll
Copy link
Owner

crswll commented Jan 5, 2024

Oh it's all good sorry if it sounds otherwise! Don't think you're backseat driving at all.

I mean like if the base theme has colors: { a: '#f00', b: '#0f0' } the following themes can only have a and/or b but not c... well at least in my brain. I guess that's technically not true but it's safer unless we want to nest themes instead of have a list.

@LandonSchropp
Copy link
Author

Ah, I see what you were going for. I think this works. It uses generics to pass through the base theme config to the other types, ensuring that the other configs are always a deep partial of it.

The only downside of this approach I've found so far is that the error message isn't necessarily the easiest to understand.

@crswll
Copy link
Owner

crswll commented Jan 9, 2024

Got started on this. Getting pretty tripped up using it with Tailwind's plugin.withOption(...) API so will need to figure that out. Definitely in the works though.

@crswll
Copy link
Owner

crswll commented Jan 15, 2024

Had some time today and sit and work on this. I have to get the tooling set up around it but think the hard part is done. I still can't get the types passed properly to plugin.withOptions<...>(plugin, extend) and might just need to make the plugin a preset instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants