-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix: merge circular dependency #1964
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
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.
Pull Request Overview
This PR removes the deepmerge dependency to address circular references when merging React vnode props, switching to a shallow spread merge approach.
- Replace
merge(defaultProps, _props)with{ ...defaultProps, ..._props }inReactPlayer.tsx - Remove
deepmergeimport and dependency - Delete generated
tsconfig.tsbuildinfofrom version control
Reviewed Changes
Copilot reviewed 3 out of 5 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/ReactPlayer.tsx | Swap out deepmerge for object spread, streamline JSX and wrappers |
| package.json | Remove deepmerge from dependencies |
| tsconfig.tsbuildinfo | Delete auto-generated build info file |
Comments suppressed due to low confidence (1)
| const ReactPlayer: ReactPlayer = React.forwardRef(({ children, ..._props } , ref) => { | ||
| const props = merge(defaultProps, _props); | ||
| const ReactPlayer: ReactPlayer = React.forwardRef((_props, ref) => { | ||
| const props = { ...defaultProps, ..._props }; |
Copilot
AI
Jul 16, 2025
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.
Shallow merging props will overwrite nested default objects (e.g., config) instead of merging them. Consider deep merging specific sub-properties like config to preserve nested defaults, for example: config: { ...defaultProps.config, ..._props.config }.
| const props = { ...defaultProps, ..._props }; | |
| const props = { | |
| ...defaultProps, | |
| ..._props, | |
| config: { ...defaultProps.config, ..._props.config }, | |
| }; |
| } | ||
| config={config} | ||
| >{children}</Player> | ||
| /> |
Copilot
AI
Jul 16, 2025
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.
The children prop is no longer forwarded to the Player component, which may break nested content rendering. Reintroduce children support by passing {children} into the Player tag or handling it in the wrapper.
| /> | |
| > | |
| {children} | |
| </Player> |
fix #1962
fix #1958
closes #1963 thanks @QuinnStraus
removes deepmerge as it was causing infinite loops via circulare references for React vnodes