chore: revert ts type changes and use smaller way #51
Conversation
yoyo837
changed the title
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Walkthrough在 Changes
Sequence Diagram(s)sequenceDiagram
participant App as 使用处
participant Wrapper as makeImmutable / responseImmutable
participant Inner as 原始 Component
rect #f0f7ff
App->>Wrapper: 渲染 WrappedComponent(props, ref)
note right of Wrapper #e8f2ff: forwardRef 封装\n(接收 ref 并传入 Inner)
Wrapper->>Inner: 调用 Inner(props, ref)
Inner-->>Wrapper: 返回 React 元素
Wrapper-->>App: 返回 React 元素
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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 |
Summary of ChangesHello @yoyo837, 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 focuses on a chore-related task to refine and simplify the TypeScript definitions and usage within the 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
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #51 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 4 4
Lines 91 90 -1
Branches 26 24 -2
=========================================
- Hits 91 90 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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 simplifies the TypeScript types in Immutable.tsx by removing more specific types in favor of any and type assertions. While this makes the code shorter, it significantly reduces type safety for components using these HOCs. My review focuses on the implications of this trade-off, highlighting the loss of compile-time checks for props and inaccurate return type assertions. I've also pointed out a minor typo in a comment.
src/Immutable.tsx
Outdated
| const refAble = supportRef(Component); | ||
|
|
||
| const ImmutableComponent = (props: ImmutableProps<T>, ref: React.Ref<any>) => { | ||
| const ImmutableComponent = function (props: any, ref: any) { |
There was a problem hiding this comment.
Using any for props and ref removes type safety for components wrapped with makeImmutable. The previous approach using a typed props object was more robust as it ensured that the props passed to the wrapped component were correctly typed. With any, we lose compile-time checks and editor support (like autocompletion and type hints) for props, which can lead to runtime errors. It would be highly beneficial to restore stronger typing here to protect consumers of this HOC.
src/Immutable.tsx
Outdated
| return refAble | ||
| ? (React.forwardRef(ImmutableComponent) as React.ComponentType<React.ComponentProps<T>>) | ||
| : (ImmutableComponent as unknown as React.ComponentType<React.ComponentProps<T>>); | ||
| return refAble ? React.forwardRef(ImmutableComponent) as unknown as T : (ImmutableComponent as T); |
There was a problem hiding this comment.
Casting the returned component to T is not type-safe. The HOC returns a new wrapper component, not the original component T. While the wrapper accepts the same props, it's a different component. This inaccurate typing can be misleading and might hide issues, for example if static properties of the original component are expected to be present on the wrapped component. The previous return type React.ComponentType<React.ComponentProps<T>> was more accurate and safer.
src/Immutable.tsx
Outdated
| const refAble = supportRef(Component); | ||
|
|
||
| const ImmutableComponent = (props: ImmutableProps<T>, ref: React.Ref<any>) => { | ||
| const ImmutableComponent = function (props: any, ref: any) { |
There was a problem hiding this comment.
Similar to makeImmutable, using any here for props and ref in responseImmutable also removes type safety. This can make the components harder to use correctly and more prone to bugs. It's highly recommended to use specific types for props to provide a better developer experience for users of this function.
src/Immutable.tsx
Outdated
| return refAble | ||
| ? (React.memo(React.forwardRef(ImmutableComponent), propsAreEqual) as React.ComponentType< | ||
| React.ComponentProps<T> | ||
| >) | ||
| : (React.memo(ImmutableComponent, propsAreEqual) as unknown as React.ComponentType< | ||
| React.ComponentProps<T> | ||
| >); | ||
| ? React.memo(React.forwardRef(ImmutableComponent), propsAreEqual) as unknown as T | ||
| : (React.memo(ImmutableComponent, propsAreEqual) as unknown as T); |
There was a problem hiding this comment.
Similar to makeImmutable, casting the memoized component to T is not type-safe. The returned component is a new wrapper component created by React.memo and React.forwardRef, not the original component T. This can lead to incorrect assumptions by the consumer of responseImmutable. A more accurate return type would better describe the returned component and avoid misleading the type system.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Immutable.tsx(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Immutable.tsx (1)
src/index.ts (1)
useImmutableMark(14-14)
src/Immutable.tsx
Outdated
| return refAble | ||
| ? (React.forwardRef(ImmutableComponent) as React.ComponentType<React.ComponentProps<T>>) | ||
| : (ImmutableComponent as unknown as React.ComponentType<React.ComponentProps<T>>); | ||
| return refAble ? React.forwardRef(ImmutableComponent) as unknown as T : (ImmutableComponent as T); |
There was a problem hiding this comment.
类型断言到 T 违背静态属性契约
这里把 React.forwardRef/React.memo 返回的新组件硬断言成原始的 T，但我们并没有把 Component 上的静态成员（自定义静态方法、defaultProps 扩展字段等）复制过去。结果是类型层面仍能访问这些静态属性，运行时却得到 undefined，一旦调用就会抛错。这是对外 API 的破坏。建议至少恢复之前返回 React.ComponentType<React.ComponentProps<T>> 的写法，或者引入 hoist-non-react-statics 之类的方案，在断言之前把静态成员完整同步过去，再返回 T。目前的实现必须修正后才能合并。
Also applies to: 97-98
|
@gemini-code-assist CR again |
There was a problem hiding this comment.
Code Review
这个 PR 旨在简化 HOC 的类型定义，这是一个很好的目标。将返回类型从 React.ComponentType<React.ComponentProps<T>> 更改为 T 能够更好地保留原始组件的类型（包括静态属性），这是一个很大的改进。代码的重构，例如在 responseImmutable 中对 React.memo 的调用，也使代码更简洁。
然而，为了移除一些类型断言（as any），ImmutableComponent 的 props 和 ref 被改成了 any 类型。这虽然达到了目的，但也牺牲了类型安全，是一种倒退。我建议使用更精确的类型，即使这意味着需要处理一些由此暴露出来的类型不匹配问题。我在代码中留下了具体的建议。
| const refAble = supportRef(Component); | ||
|
|
||
| const ImmutableComponent = (props: ImmutableProps<T>, ref: React.Ref<any>) => { | ||
| const ImmutableComponent: React.ForwardRefRenderFunction<any, any> = (props, ref) => { |
There was a problem hiding this comment.
将 ImmutableComponent 的类型定义为 React.ForwardRefRenderFunction<any, any> 会导致 props 和 ref 失去类型信息，降低了类型安全性。虽然这可以避免在后续代码中使用 as any，但更好的做法是提供更精确的类型。
我建议使用 React.ElementRef<T> 和 React.ComponentPropsWithoutRef<T> 来精确地定义 ref 和 props 的类型。
请注意，这个改动可能会暴露出 shouldTriggerRender 函数的类型不匹配问题。它的类型 CompareProps<T> 期望的参数是 React.ComponentProps<T>，而 ImmutableComponent 的 props 类型是 React.ComponentPropsWithoutRef<T>。根本原因可能是 CompareProps<T> 的定义，它或许应该使用 React.ComponentPropsWithoutRef<T>。考虑到这是一个更深层次的问题，一个临时的解决方案可能是在调用 shouldTriggerRender 时进行类型断言，但这正是本次 PR 想要移除的。最好的方式是修复 CompareProps<T> 的类型定义。
| const ImmutableComponent: React.ForwardRefRenderFunction<any, any> = (props, ref) => { | |
| const ImmutableComponent: React.ForwardRefRenderFunction< | |
| React.ElementRef<T>, | |
| React.ComponentPropsWithoutRef<T> | |
| > = (props, ref) => { |
| const refAble = supportRef(Component); | ||
|
|
||
| const ImmutableComponent = (props: ImmutableProps<T>, ref: React.Ref<any>) => { | ||
| const ImmutableComponent: React.ForwardRefRenderFunction<any, any> = (props, ref) => { |
There was a problem hiding this comment.
与 makeImmutable 中的情况类似，这里使用 React.ForwardRefRenderFunction<any, any> 也导致了 props 和 ref 的类型安全性的丧失。
建议使用更精确的类型来定义 ImmutableComponent。
同样，请注意这个改动可能会影响 propsAreEqual 的类型检查。propsAreEqual 的类型是 CompareProps<T>，它期望的参数是 React.ComponentProps<T>。而 React.memo 会传入 ImmutableComponent 的 props，其类型是 React.ComponentPropsWithoutRef<T>，这会导致类型不匹配。这个问题也源于 CompareProps<T> 的定义。
| const ImmutableComponent: React.ForwardRefRenderFunction<any, any> = (props, ref) => { | |
| const ImmutableComponent: React.ForwardRefRenderFunction< | |
| React.ElementRef<T>, | |
| React.ComponentPropsWithoutRef<T> | |
| > = (props, ref) => { |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
yoyo837
force-pushed
the
ts-type-smaller
branch
from
a5add8e to
fd65fbe
Compare
Summary by CodeRabbit