-
Notifications
You must be signed in to change notification settings - Fork 11
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
Fix useTheme hook type #23
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot! 👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I jumped the gun with my review a bit, sorry about that. This change is in the right direction but not quite 100% correct since currently the useTheme
hook in Stitches Native actually returns the pure theme token values and not the so called theme definition object that this added type describes.
The reason for this is that I didn't see the benefit of returning the definition object:
{ token, scale, value, toString }
when in most cases you just want to use the pure value when access the theme:
const theme = useTheme();
const x = theme.colors.white; // <---- this is a string not an object
const y = theme.space[1]; // <---- this is a number not an object
If we changed the |
I see. So the root problem is the type of The // ...
theme: string &
{
[Scale in keyof Theme]: {
[Token in keyof Theme[Scale]]: ThemeUtil.Token<
Extract<Token, string | number>,
Theme[Scale][Token],
Extract<Scale, string | void>
>;
};
};
// ... That's make the |
I'm checking the original stitches project and noticed that |
I've noticed that theme: string &
{
[Scale in keyof Theme]: {
[Token in keyof Theme[Scale]]: Theme[Scale][Token];
};
}; |
Yeah that definitely mostly solves the problem 👍🏻 Instead of mapping the
is exactly the same as just One remaining problem that this doesn't solve is that token aliases inside the theme will have a
But maybe that is a trade-off that we can make to satisfy the most common use case 🤔 |
Okay I was able to create a utility type that casts token aliases as type IsPrefixed<T extends string = ''> = T extends `${infer Head}${infer Tail}`
? Head extends '$'
? 'true'
: 'false'
: 'false';
export type TokenValue<Value extends string | number> = Value extends number
? number
: IsPrefixed<Value> extends 'true'
? unknown
: string; We can use this type for
In order to get this to work the user needs to assert the token alias {
colors: {
blue100: '#ab9cf7', // <-- this will be `string`
primary: '$blue100' as const, // <-- this will be `unknown` when accessing from `useTheme`
},
space: {
1: 8, // <-- this will be `number`
foo: '$1' as const, // <-- this will be `unknown` when accessing from `useTheme`
bar: '$1', // <-- this will be incorrectly `string` since `as const` is missing
}
} I couldn't get the actual type of the token alias (not sure it's even possible in TS 🤷🏻♂️). This did not work: export type TokenValue<
Tokens extends {} = {},
Token extends keyof Tokens,
Value extends string | number = Tokens[Token]
> = Value extends number
? number
: IsPrefixed<Value> extends 'true'
? Tokens[Value]
: string; However, I think it is better to have the type to be What do you @alexaragao think? |
@Temzasse I do agree that return |
Maintaining the use of
type AliasedToken<T extends string> = T extends `${infer Head}${infer Tail}`
? Head extends "$"
? Tail
: never
: never;
theme: string &
{
[Scale in keyof Theme]: {
[Token in keyof Theme[Scale]]:
// Check if token value is a string
Theme[Scale][Token] extends string
? (
// Check if token value is an alias, never means 'no'
AliasedToken<Theme[Scale][Token]> extends never
// if not, return string
? string
// if so, return token value type
: Theme[Scale][AliasedToken<Theme[Scale][Token]>]
)
// if not, return token value type
: Theme[Scale][Token];
};
}; The type is kinda ugly, but prevents the EDIT: My solution to prevent empty token aliases (use only "$") had a bug. Now it returns |
@alexaragao oh wow you managed to extract the actual type for the alias, great job! 🤩 🙌🏻 The second code snippet in your message should be How do you want to proceed with these changes? I could apply your type proposal in the main branch and also write some guidance about the need for PS: I'm quite sure there is no way to avoid the |
5fb9943
to
7f5a39e
Compare
Thanks 😄! I overwrote my commit with the proposed changes, everything should be fine now. I only changed the type of NOTE: I tried to "hard cast" the aliased token using |
This is now fixed in the Big thanks for the fix @alexaragao! 👍🏻 |
I'm solving my own issue (#22), this changes did work for me in my project.