Skip to content

fix: prevent infinite recursion in deepmerge for other react props#1963

Closed
QuinnStraus wants to merge 2 commits intocookpete:masterfrom
QuinnStraus:master
Closed

fix: prevent infinite recursion in deepmerge for other react props#1963
QuinnStraus wants to merge 2 commits intocookpete:masterfrom
QuinnStraus:master

Conversation

@QuinnStraus
Copy link
Copy Markdown

@QuinnStraus QuinnStraus commented Jul 16, 2025

Addresses deepmerge issues when passing react components as props (particularly the fallback, playIcon, light, and wrapper props).
Extracts relevant keys from props and defaultProps objects and handles merging with defaultProps manually for those props.

Fix for #1962 and #1958

@luwes luwes requested a review from Copilot July 16, 2025 20:02
Copy link
Copy Markdown

Copilot AI left a 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 fixes infinite recursion issues in the deepmerge function when React components are passed as props, specifically addressing problems with fallback, playIcon, light, and wrapper props.

  • Extracts React component props from destructuring to prevent deepmerge from processing them
  • Implements manual prop merging using nullish coalescing for React elements
  • Updates references throughout the component to use the manually merged props

Comment on lines +57 to +60
const playIcon = _playIcon ?? defaultPlayIcon;
const light = _light ?? defaultLight;
const fallback = _fallback ?? defaultFallback;
const wrapper = _wrapper ?? defaultWrapper;
Copy link

Copilot AI Jul 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Consider extracting the manual prop merging logic into a helper function to reduce repetition and improve maintainability. The four lines of nullish coalescing follow the same pattern.

Suggested change
const playIcon = _playIcon ?? defaultPlayIcon;
const light = _light ?? defaultLight;
const fallback = _fallback ?? defaultFallback;
const wrapper = _wrapper ?? defaultWrapper;
const mergeProp = <T>(overriddenValue: T, defaultValue: T): T => overriddenValue ?? defaultValue;
const playIcon = mergeProp(_playIcon, defaultPlayIcon);
const light = mergeProp(_light, defaultLight);
const fallback = mergeProp(_fallback, defaultFallback);
const wrapper = mergeProp(_wrapper, defaultWrapper);

Copilot uses AI. Check for mistakes.
Comment on lines +99 to 104
const { style, width, height } = props;
const config = props.config?.[player.key as keyof ReactPlayerProps['config']];

return (
<Player
{...props}
Copy link

Copilot AI Jul 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The props are being spread with {...props} and then individual props are explicitly passed. This could lead to prop override issues if the same prop exists in both the spread and explicit props. Consider removing these props from the spread object or ensuring proper precedence.

Suggested change
const { style, width, height } = props;
const config = props.config?.[player.key as keyof ReactPlayerProps['config']];
return (
<Player
{...props}
const { style, width, height, playIcon, light, fallback, wrapper, ref, slot, className, config: propsConfig, ...restProps } = props;
const config = propsConfig?.[player.key as keyof ReactPlayerProps['config']];
return (
<Player
{...restProps}

Copilot uses AI. Check for mistakes.
} = defaultProps;
const props = merge(_defaultProps, _props);

// deepmerge does not handle React elements, so we need to extract and merge them manually
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the contribution! I'm not fully following yet, React elements are just objects (vnodes) so pretty sure deepmerge does handle them.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh it's similar to https://github.com/cookpete/react-player/pull/1933/files

so deepmerge doesn't handle circular references
TehShrike/deepmerge#207

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok I've repro'd it https://codesandbox.io/p/devbox/6zg6sf?file=%2Fapp%2Fplayer.tsx%3A18%2C1

gonna take another route though, there is no need for deepmerge anymore so we can remove it

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah dug into the source a little bit and seems like react nodes sometimes contain circular references which get it stuck in a loop. I also noticed the defaultProps did not contain any nested objects so the deepmerge seemed unnecessary anyway, and regardless deeply merging react nodes seems like it would be undesired anyway.

Thanks for your quick responses!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants