fix: undefined error when consuming cssvar #200
Conversation
Summary of ChangesHello @Wxh16144, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses and fixes an Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses an issue with cssVar being undefined. The core of the fix is the introduction of a new useCustomToken hook, which generates CSS variables for custom tokens by using internal APIs from Ant Design. The theme is then updated to include these variables. While the approach seems to solve the problem, my review identified several areas for improvement. There are multiple uses of any type assertions that compromise type safety, and a significant reliance on Ant Design's internal APIs, which could lead to issues with future updates. I also found a potential runtime error in one of the demo files.
| box: { | ||
| width: 32, | ||
| height: 32, | ||
| background: { ...(cssVar as any) }[tokenName!] || 'pink', |
There was a problem hiding this comment.
The non-null assertion tokenName! is unsafe because tokenName is an optional property. If tokenName is undefined, this will cause a runtime error. A safer approach is to conditionally access the property only when tokenName is defined.
| background: { ...(cssVar as any) }[tokenName!] || 'pink', | |
| background: (tokenName && cssVar && (cssVar as any)[tokenName]) || 'pink', |
| @@ -0,0 +1,35 @@ | |||
| import { CustomToken } from '@/types'; | |||
| import { useCacheToken } from '@ant-design/cssinjs'; | |||
| import { useToken as useInternalToken } from 'antd/lib/theme/internal'; | |||
There was a problem hiding this comment.
Using useToken from antd/lib/theme/internal is risky as it's an internal Ant Design API. It can break without warning in future antd versions. If possible, it would be better to rely on public APIs to ensure long-term stability.
| return { ...defaultCustomToken, ...customTokenOrFn }; | ||
| }, [defaultCustomToken, customTokenOrFn, token, appearance]); | ||
|
|
||
| const { token: cssVariableToken, cssVarCls, hashId } = useCustomToken(customToken as any); |
There was a problem hiding this comment.
Casting customToken to any bypasses type checking. It would be safer to ensure that customToken's type is compatible with the CustomToken type expected by useCustomToken. Consider adding a generic constraint like T extends CustomToken where T is defined, for example in ThemeProviderProps.
|
|
||
| const theme: Theme = { | ||
| ...token, | ||
| ...(customToken as any), |
There was a problem hiding this comment.
Casting customToken to any before spreading it into the theme object weakens type safety. The root cause seems to be that the generic T for custom tokens is not constrained. Constraining it to extend CustomToken would improve type safety here and in other places.
| const [token, hashId, realToken] = useCacheToken<CustomToken>(theme, [], { | ||
| salt: 'antd-style', | ||
| override: customToken || {}, | ||
| getComputedToken: (_, override) => override as any, |
There was a problem hiding this comment.
Casting override to any weakens type safety. If the types are set up correctly, this cast should not be necessary. It would be better to adjust the types so that override can be returned directly, for instance by ensuring the override parameter's type is a full CustomToken rather than a partial one if that's the case.
1 participant
maybe fix: #199