Conversation
There was a problem hiding this comment.
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 }; |
There was a problem hiding this comment.
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> | ||
| /> |
There was a problem hiding this comment.
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