Skip to content

Commit

Permalink
Assign merged props to a const instead of modifying initialProps (#2718)
Browse files Browse the repository at this point in the history
  • Loading branch information
KenanYusuf authored Jan 16, 2024
1 parent 4556f3d commit ab8d636
Show file tree
Hide file tree
Showing 4 changed files with 31 additions and 19 deletions.
7 changes: 7 additions & 0 deletions .changeset/angry-pens-provide.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"victory-chart": patch
"victory-native": patch
"victory-stack": patch
---

Assign merged props to a const instead of modifying initialProps
19 changes: 11 additions & 8 deletions packages/victory-chart/src/victory-chart.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,14 @@ const defaultProps = {
};

const VictoryChartImpl: React.FC<VictoryChartProps> = (initialProps) => {
initialProps = { ...defaultProps, ...initialProps };
const propsWithDefaults = React.useMemo(
() => ({ ...defaultProps, ...initialProps }),
[initialProps],
);
const role = "chart";
const { getAnimationProps, setAnimationState, getProps } =
Hooks.useAnimationState();
const props = getProps(initialProps);
const props = getProps(propsWithDefaults);

const modifiedProps = Helpers.modifyProps(props, fallbackProps, role);
const {
Expand Down Expand Up @@ -154,7 +157,7 @@ const VictoryChartImpl: React.FC<VictoryChartProps> = (initialProps) => {
{},
containerComponent.props,
containerProps,
UserProps.getSafeUserProps(initialProps),
UserProps.getSafeUserProps(propsWithDefaults),
);
return React.cloneElement(containerComponent, defaultContainerProps);
}
Expand All @@ -164,23 +167,23 @@ const VictoryChartImpl: React.FC<VictoryChartProps> = (initialProps) => {
standalone,
containerComponent,
containerProps,
initialProps,
propsWithDefaults,
]);

const events = React.useMemo(() => {
return Wrapper.getAllEvents(props);
}, [props]);

const previousProps = Hooks.usePreviousProps(initialProps);
const previousProps = Hooks.usePreviousProps(propsWithDefaults);

React.useEffect(() => {
// This is called before dismount to keep state in sync
return () => {
if (initialProps.animate) {
setAnimationState(previousProps, initialProps);
if (propsWithDefaults.animate) {
setAnimationState(previousProps, propsWithDefaults);
}
};
}, [setAnimationState, previousProps, initialProps]);
}, [setAnimationState, previousProps, propsWithDefaults]);

if (!isEmpty(events)) {
return (
Expand Down
4 changes: 2 additions & 2 deletions packages/victory-native/src/helpers/wrap-core-component.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ import React from "react";
*/
export const wrapCoreComponent = ({ Component, defaultProps }) => {
const WrappedComponent = (props) => {
props = { ...defaultProps, ...props };
return <Component {...props} />;
const propsWithDefaults = { ...defaultProps, ...props };
return <Component {...propsWithDefaults} />;
};

/**
Expand Down
20 changes: 11 additions & 9 deletions packages/victory-stack/src/victory-stack.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -61,13 +61,15 @@ const defaultProps = {
};

const VictoryStackBase = (initialProps: VictoryStackProps) => {
// eslint-disable-next-line no-use-before-define
const { role } = VictoryStack;
initialProps = { ...defaultProps, ...initialProps };
const propsWithDefaults = React.useMemo(
() => ({ ...defaultProps, ...initialProps }),
[initialProps],
);
const { setAnimationState, getAnimationProps, getProps } =
Hooks.useAnimationState();

const props = getProps(initialProps);
const props = getProps(propsWithDefaults);

const modifiedProps = Helpers.modifyProps(props, fallbackProps, role);
const {
Expand Down Expand Up @@ -134,8 +136,8 @@ const VictoryStackBase = (initialProps: VictoryStackProps) => {
name,
]);
const userProps = React.useMemo(
() => UserProps.getSafeUserProps(initialProps),
[initialProps],
() => UserProps.getSafeUserProps(propsWithDefaults),
[propsWithDefaults],
);

const container = React.useMemo(() => {
Expand All @@ -161,16 +163,16 @@ const VictoryStackBase = (initialProps: VictoryStackProps) => {
return Wrapper.getAllEvents(props);
}, [props]);

const previousProps = Hooks.usePreviousProps(initialProps);
const previousProps = Hooks.usePreviousProps(propsWithDefaults);

React.useEffect(() => {
// This is called before dismount to keep state in sync
return () => {
if (initialProps.animate) {
setAnimationState(previousProps, initialProps);
if (propsWithDefaults.animate) {
setAnimationState(previousProps, propsWithDefaults);
}
};
}, [setAnimationState, previousProps, initialProps]);
}, [setAnimationState, previousProps, propsWithDefaults]);

if (!isEmpty(events)) {
return (
Expand Down

1 comment on commit ab8d636

@vercel
Copy link

@vercel vercel bot commented on ab8d636 Jan 16, 2024

Choose a reason for hiding this comment

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

Please sign in to comment.