-
Notifications
You must be signed in to change notification settings - Fork 160
feat: polish types to be compatible with React 18.3 #4735
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
Conversation
LLM Analysis of PR ChangesSummaryThis PR updates the React and React DOM dependencies to be compatible with React 18.3. It also adds type casting to React.RefObjects in several components to resolve TypeScript errors related to stricter type checking in React 18. Key Points to Review
Style & Consistency
|
3b98f1e
to
b21d412
Compare
Deploying orbit with
|
Latest commit: |
3a03910
|
Status: | ✅ Deploy successful! |
Preview URL: | https://2fa98834.orbit.pages.dev |
Branch Preview URL: | https://sarka-react-upgrade-fix.orbit.pages.dev |
Size Change: +39 B (+0.01%) Total Size: 464 kB
ℹ️ View Unchanged
|
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.
Key Issues
The incorrect return type React.RefObject<T>
that doesn't account for null values could lead to runtime errors if not handled properly. Suppressing type errors with // @ts-expect-error
without proper checks can cause runtime issues, especially if scrollingElementRef
is not a valid React ref object. Removing null
from ref types is unsafe as React refs can be null before mounting or if the element is unmounted, potentially causing runtime errors.
// @ts-expect-error TODO | ||
// eslint-disable-next-line no-param-reassign | ||
scrollingElementRef.current = node; |
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.
🐛 Possible Bug
The // @ts-expect-error
suppresses a type error without proper type checking. If scrollingElementRef
is not a valid React ref object with a current
property, this assignment could cause runtime errors. Consider adding proper type guards or using MutableRefObject
type.
// @ts-expect-error TODO | |
// eslint-disable-next-line no-param-reassign | |
scrollingElementRef.current = node; | |
if (scrollingElementRef && 'current' in scrollingElementRef) { | |
scrollingElementRef.current = node; | |
} |
@@ -21,7 +21,7 @@ export type Props = Common.Globals & | |||
closable & { | |||
readonly size?: Size; | |||
readonly children: React.ReactNode; | |||
readonly triggerRef?: React.RefObject<HTMLElement | null>; | |||
readonly triggerRef?: React.RefObject<HTMLElement>; |
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.
🐛 Possible Bug
Removing null
from RefObject
and Ref
types is unsafe as React refs can be null before the component mounts or if the referenced element is unmounted. This can lead to runtime errors when accessing these refs.
readonly triggerRef?: React.RefObject<HTMLElement>; | |
readonly triggerRef?: React.RefObject<HTMLElement | null>; |
@@ -21,7 +21,7 @@ export type Props = Common.Globals & | |||
closable & { | |||
readonly size?: Size; | |||
readonly children: React.ReactNode; | |||
readonly triggerRef?: React.RefObject<HTMLElement | null>; | |||
readonly triggerRef?: React.RefObject<HTMLElement>; | |||
readonly lockScrolling?: boolean; | |||
readonly scrollingElementRef?: React.Ref<HTMLElement | null>; |
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.
🐛 Possible Bug
The scrollingElementRef
should allow for null values as the ref might not be initialized immediately or could become null during component lifecycle.
readonly scrollingElementRef?: React.Ref<HTMLElement | null>; | |
readonly scrollingElementRef?: React.Ref<HTMLElement | null>; |
b21d412
to
3ec7c79
Compare
Storybook staging is available at https://kiwicom-orbit-sarka-react-upgrade-fix.surge.sh Playroom staging is available at https://kiwicom-orbit-sarka-react-upgrade-fix.surge.sh/playroom |
f42a789
to
eb8a6fe
Compare
eb8a6fe
to
ba76d33
Compare
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.
Key Issues
The use of @ts-expect-error
without proper type checking can lead to runtime errors, and the assignment of a parameter typed as any
to ref.current
without validation may cause type-related bugs.
ba76d33
to
c8cd84c
Compare
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.
Key Issues
The use of any
type for the callback parameter bypasses TypeScript's type checking, risking runtime errors; it should be explicitly typed to ensure type safety. Suppressing type mismatches with @ts-expect-error
without proper validation can lead to runtime errors if types are not as expected, necessitating type validation or assertions.
// @ts-expect-error TODO | ||
// eslint-disable-next-line no-param-reassign | ||
scrollingElementRef.current = node; |
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.
🐛 Possible Bug
The @ts-expect-error
comment suppresses a type mismatch between scrollingElementRef.current
and node
without proper type validation. This could lead to runtime errors if node
is not of the expected type that scrollingElementRef.current
should accept. Consider adding type validation or using proper type assertions.
// @ts-expect-error TODO | |
// eslint-disable-next-line no-param-reassign | |
scrollingElementRef.current = node; | |
if (scrollingElementRef.current !== null) { | |
scrollingElementRef.current = node; | |
} |
c8cd84c
to
ce8808f
Compare
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.
Key Issues
Improper use of @ts-expect-error
can lead to runtime errors due to unaddressed type issues in scrollingElementRef
. Using any
type for parameters bypasses TypeScript's type checking, risking type-related bugs; maintaining generic types ensures type safety.
// @ts-expect-error TODO | ||
// eslint-disable-next-line no-param-reassign | ||
scrollingElementRef.current = node; |
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.
🐛 Possible Bug
Using @ts-expect-error
without addressing the underlying type issue is dangerous. The scrollingElementRef
might not have the expected structure, which could lead to runtime errors when accessing .current
. Consider properly typing the ref or handling the case where the ref structure is different.
// @ts-expect-error TODO | |
// eslint-disable-next-line no-param-reassign | |
scrollingElementRef.current = node; | |
if (scrollingElementRef && 'current' in scrollingElementRef) { | |
// eslint-disable-next-line no-param-reassign | |
scrollingElementRef.current = node; | |
} |
ce8808f
to
e4827a7
Compare
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.
Key Issues
The use of @ts-expect-error
is hiding a potential type mismatch in scrollingElementRef.current
, which could lead to runtime errors; proper typing of the ref is necessary to ensure type safety.
// @ts-expect-error TODO | ||
// eslint-disable-next-line no-param-reassign | ||
scrollingElementRef.current = node; |
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.
🐛 Possible Bug
The @ts-expect-error
suppression is masking a potential type mismatch when assigning to scrollingElementRef.current
. This could lead to runtime errors if the ref object doesn't match the expected type. Instead, we should properly type the ref to ensure type safety.
// @ts-expect-error TODO | |
// eslint-disable-next-line no-param-reassign | |
scrollingElementRef.current = node; | |
scrollingElementRef.current = node as HTMLElement; |
e4827a7
to
0ec4742
Compare
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.
Key Issues
Improper use of @ts-expect-error
bypasses TypeScript's type checking, risking runtime errors due to unvalidated assumptions about ref.current
and scrollingElementRef
properties.
// @ts-expect-error TODO | ||
// eslint-disable-next-line no-param-reassign | ||
scrollingElementRef.current = node; |
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.
🐛 Possible Bug
Using @ts-expect-error
without proper validation or type checking could lead to runtime errors if scrollingElementRef
is not a valid React ref object. The code assumes it has a current
property but doesn't validate this assumption.
// @ts-expect-error TODO | |
// eslint-disable-next-line no-param-reassign | |
scrollingElementRef.current = node; | |
if (!scrollingElementRef || typeof scrollingElementRef !== 'object' || !('current' in scrollingElementRef)) { | |
console.warn('Invalid scrollingElementRef provided to Modal'); | |
return; | |
} | |
// eslint-disable-next-line no-param-reassign | |
scrollingElementRef.current = node; |
0ec4742
to
3a03910
Compare
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.
Key Issues
The use of the any
type for the value
parameter bypasses TypeScript's type checking, risking runtime errors; it should be typed as T
to ensure type safety.
This Pull Request meets the following criteria:
For new components:
d.ts
files and are exported inindex.d.ts
✨
Description by Callstackai
This PR updates the type definitions for compatibility with React 18.3, modifies package dependencies, and refines the usage of refs in various components.
Diagrams of code changes
Files Changed
This PR includes files in programming languages that we currently do not support. We have not reviewed files with the extensions
.json
. See list of supported languages.