fix: Revert #1244 to address regression w/ Button and DropdownItem #1298
Conversation
…n the button `as` prop (themesberg#1244)" This reverts commit a6698d4.
|
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1298 +/- ##
==========================================
- Coverage 99.54% 95.54% -4.01%
==========================================
Files 163 218 +55
Lines 6621 9691 +3070
Branches 401 560 +159
==========================================
+ Hits 6591 9259 +2668
- Misses 30 432 +402 ☔ View full report in Codecov by Sentry. |
|
@coderabbitai review |
WalkthroughThis update focuses on improving type inference and component refactoring within a React project. It addresses Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (11)
- components/homepage/components-section.tsx (1 hunks)
- components/homepage/hero-section/hero-section.tsx (1 hunks)
- components/homepage/react-section.tsx (1 hunks)
- examples/button/button.polymorph.tsx (1 hunks)
- examples/dropdown/dropdown.customItem.tsx (1 hunks)
- src/components/Button/Button.tsx (3 hunks)
- src/components/Button/ButtonBase.tsx (1 hunks)
- src/components/Dropdown/Dropdown.spec.tsx (1 hunks)
- src/components/Dropdown/Dropdown.tsx (2 hunks)
- src/components/Dropdown/DropdownItem.tsx (1 hunks)
- src/helpers/generic-forward-ref.ts (1 hunks)
Additional comments: 12
src/helpers/generic-forward-ref.ts (2)
- 6-9: The use of
@typescript-eslint/ban-typesand{}in the type definition ofFixedForwardRefis necessary for the functionality but could benefit from a comment explaining why this approach was chosen and the implications it might have on type safety.- 11-11: The implementation of
genericForwardRefas a type-casted version offorwardRefis a clever solution to enable generic component forwarding. This change looks good and should facilitate the refactoring of components likeButtonandDropdownItem.src/components/Button/ButtonBase.tsx (1)
- 9-15: The refactoring of
ButtonBaseintoButtonBaseComponentand its export usinggenericForwardRefis a significant improvement in terms of type safety and component flexibility. Ensure that the use of generics and props spreading is thoroughly tested to prevent any potential issues with type inference.Verification successful
Given the lack of output from the ast-grep command, it's necessary to explore alternative methods to verify the usage of
ButtonBaseComponentand its type safety. I will now use ripgrep (rg) to search for textual occurrences ofButtonBaseComponentin the codebase to ensure a more comprehensive search.
The search results indicate that
ButtonBaseComponentis defined and exported withinsrc/components/Button/ButtonBase.tsx, with no direct evidence of its usage with variousasprops across the codebase. This limits our ability to verify the type safety and flexibility ofButtonBaseComponentin different usage scenarios directly. However, the importance of thorough testing, as mentioned in the initial review comment, to ensure type safety and flexibility, especially when using generics and props spreading, remains critical. It's advisable to conduct comprehensive testing, particularly in scenarios whereButtonBaseComponentmight be used with differentasprops in the future.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the usage of ButtonBaseComponent with various `as` props and ensure type safety. ast-grep --lang typescript --pattern $'ButtonBaseComponent<$$_>({ as: $$_, ... })'Length of output: 83
Script:
#!/bin/bash # Search for textual occurrences of ButtonBaseComponent to verify its usage. rg "ButtonBaseComponent"Length of output: 228
src/components/Dropdown/DropdownItem.tsx (2)
- 17-24: The simplification of the
DropdownItemPropsinterface is a positive change that enhances readability and maintainability. However, ensure that this simplification does not limit the component's flexibility or its ability to be used in various contexts.- 26-59: The refactoring of the
DropdownItemcomponent, including the use of context hooks and theme customization, is well-implemented. It's important to perform performance testing, especially for the deep merging of themes, to ensure that there are no significant impacts on rendering times.src/components/Button/Button.tsx (4)
- 1-4: The imports have been updated to include
ComponentPropsWithoutRef,ElementType,ForwardedReffrom 'react', andgenericForwardReffrom '../../helpers/generic-forward-ref'. Ensure that these changes align with the new implementation requirements and that there are no unused imports.- 70-87: The
ButtonPropstype has been refactored to includeComponentPropsWithoutRef<T>. This change enhances the type safety and flexibility of theButtoncomponent by allowing it to accept all props valid for the element type specified byasprop, excludingref. Ensure that all usages ofButtonPropsthroughout the codebase are updated to reflect this change.- 89-163: The
ButtonComponentFnfunction has been updated to use a generic type parameter andForwardedRef<T>. This change improves the component's ability to forward refs correctly and supports generic type usage. Additionally, the function now directly returns theButtonBasecomponent wrapped in JSX, which simplifies the component structure. Ensure that the ref forwarding functionality is tested, especially in scenarios where theButtoncomponent is used with differentasprops.- 167-169: The addition of
genericForwardRefand the creation ofButtonComponentusing it is a significant improvement. This approach allows for forwarding refs in a type-safe manner for generic components. Verify thatgenericForwardRefis implemented correctly and that it handles all edge cases, such as when the ref is not provided or when the component is used with different element types.src/components/Dropdown/Dropdown.spec.tsx (1)
- 170-170: The typo in the test description has been corrected from "Dropdownn.Item" to "Dropdown.Item". This change improves the readability and accuracy of the test descriptions. Ensure that similar typos are corrected throughout the test suite to maintain consistency and clarity.
src/components/Dropdown/Dropdown.tsx (2)
- 13-13: The import of
RefCallbackfrom 'react' has been added. This type is used to specify a callback function that receives a DOM element or a component instance as its argument. Ensure that this import is used correctly in the context of ref forwarding within theDropdowncomponent.- 101-107: The
refprop in theButtoncomponent within theDropdowncomponent has been updated to userefs.setReference as RefCallback<'button'>. This change ensures that the ref is correctly typed and forwarded, improving the component's interoperability and type safety. Verify that this change does not introduce any regressions in the behavior of theDropdowncomponent, especially in scenarios where theButtoncomponent is used with different element types or in complex layouts.
|
Uh, whatever AI you’ve unleashed on this PR is pretty low-quality. All I’ve done here is ‘git revert’ 1 set of changes and partially revert another. |
Yep, I know that, but seems like the AI does not |
|
@rnicholus AI is just an tool, we still reviewing as humans. 😁 |
|
Is there anything else i can do to ensure this change is merged & released? |
| @@ -25,6 +25,7 @@ export const ComponentsSection: FC = () => { | |||
| ))} | |||
| </div> | |||
| <div className="mb-4 flex w-full justify-center text-center"> | |||
| {/* @ts-expect-error TODO: fix `as` inference */} | |||
There was a problem hiding this comment.
As you are fixing #962, is it necessary to have these @ts-expect-error comments?
There was a problem hiding this comment.
Yes, this is needed since the fix is reverting #1244.
|
@rnicholus please, no merge with main... rebase it instead! |
this can all be squashed on merge, correct? |
|
@rnicholus yes, but it is preferable to rebase with the main instead of merging it. But, anyway... I'm wondering if this PR is actually fixing the issue, and not just reverting for some broken state. Isn't better if we could address the problem with a definitive fix? What would be the pros and cons of accepting this to fix #962 and reverting the fix for #1002 and #1107? |
I'm not sure. My motivation here was to fix the typing issue without having to resort to
absolutely, but i'm not sure what that fix is as i don't really have the cycles to investigate |
|
Closed in favor of #1308 |
In #1244, the return type of
ButtonandDropdownItemwas changed toReactNode. This is not an acceptable return type for functional components. The correct return type isJSX.Element. This change fixes the regression from that PR (introduced in v0.7.3) by reverting that PR and accounting for related changes that followed that PR. Also fixes #962.Perhaps there is a way to fulfill the goals of #1244 and solve this regression, but I wasn't able to find time to address that, and #1244 seems flawed enough that perhaps it just needs to be redesigned.