Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
266 changes: 155 additions & 111 deletions src/CSSMotionList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import * as React from 'react';
import type { CSSMotionProps } from './CSSMotion';
import OriginCSSMotion, { isRefNotConsumed } from './CSSMotion';
import useIsomorphicLayoutEffect from './hooks/useIsomorphicLayoutEffect';
import type { KeyObject } from './util/diff';
import {
diffKeys,
Expand Down Expand Up @@ -67,6 +68,65 @@ export interface CSSMotionListState {
keyEntities: KeyObject[];
}

function getDerivedKeyEntities(
keys: CSSMotionListProps['keys'],
keyEntities: KeyObject[],
) {
const parsedKeyObjects = parseKeys(keys);
const mixedKeyEntities = diffKeys(keyEntities, parsedKeyObjects);

const nextKeyEntities = mixedKeyEntities.filter(entity => {
const prevEntity = keyEntities.find(({ key }) => entity.key === key);

// Remove if already mark as removed
if (
prevEntity &&
prevEntity.status === STATUS_REMOVED &&
entity.status === STATUS_REMOVE
) {
return false;
}
return true;
});

return isSameKeyEntities(keyEntities, nextKeyEntities)
? keyEntities
: nextKeyEntities;
}

function shallowEqualKeyObject(origin: KeyObject, target: KeyObject) {
if (origin === target) {
return true;
}

const originRecord = origin as Record<string, any>;
const targetRecord = target as Record<string, any>;
const originKeys = Object.keys(originRecord);
const targetKeys = Object.keys(targetRecord);

return (
originKeys.length === targetKeys.length &&
originKeys.every(
key =>
Object.prototype.hasOwnProperty.call(targetRecord, key) &&
Object.is(originRecord[key], targetRecord[key]),
)
);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
Comment thread
QDyanbing marked this conversation as resolved.

function isSameKeyEntities(origin: KeyObject[], target: KeyObject[]) {
if (origin === target) {
return true;
}

return (
origin.length === target.length &&
origin.every((entity, index) =>
shallowEqualKeyObject(entity, target[index]),
)
);
}
Comment thread
QDyanbing marked this conversation as resolved.

/**
* Generate a CSSMotionList component with config
* @param transitionSupport No need since CSSMotionList no longer depends on transition support
Expand All @@ -75,123 +135,107 @@ export interface CSSMotionListState {
export function genCSSMotionList(
transitionSupport: boolean,
CSSMotion = OriginCSSMotion,
): React.ComponentClass<CSSMotionListProps> {
class CSSMotionList extends React.Component<
CSSMotionListProps,
CSSMotionListState
> {
static defaultProps = {
component: 'div',
};

state: CSSMotionListState = {
keyEntities: [],
};

static getDerivedStateFromProps(
{ keys }: CSSMotionListProps,
{ keyEntities }: CSSMotionListState,
) {
const parsedKeyObjects = parseKeys(keys);
const mixedKeyEntities = diffKeys(keyEntities, parsedKeyObjects);

return {
keyEntities: mixedKeyEntities.filter(entity => {
const prevEntity = keyEntities.find(({ key }) => entity.key === key);

// Remove if already mark as removed
if (
prevEntity &&
prevEntity.status === STATUS_REMOVED &&
entity.status === STATUS_REMOVE
) {
return false;
}
return true;
}),
};
): React.ComponentType<CSSMotionListProps> {
const CSSMotionList: React.FC<CSSMotionListProps> = props => {
const {
component = 'div',
children,
onVisibleChanged,
onAllRemoved,
...restProps
} = props;
const { keys } = restProps;

const [keyEntities, setKeyEntities] = React.useState<KeyObject[]>(() =>
getDerivedKeyEntities(keys, []),
);
const prevKeyEntitiesRef = React.useRef<KeyObject[]>(keyEntities);
const onAllRemovedRef = React.useRef(onAllRemoved);

onAllRemovedRef.current = onAllRemoved;

let mergedKeyEntities = keyEntities;
const nextKeyEntities = getDerivedKeyEntities(keys, keyEntities);
if (!isSameKeyEntities(keyEntities, nextKeyEntities)) {
setKeyEntities(nextKeyEntities);
mergedKeyEntities = nextKeyEntities;
}

// ZombieJ: Return the count of rest keys. It's safe to refactor if need more info.
removeKey = (removeKey: React.Key) => {
this.setState(
prevState => {
const nextKeyEntities = prevState.keyEntities.map(entity => {
if (entity.key !== removeKey) return entity;
return {
...entity,
status: STATUS_REMOVED,
};
});
useIsomorphicLayoutEffect(() => {
const prevActiveCount = prevKeyEntitiesRef.current.filter(
({ status }) => status !== STATUS_REMOVED,
).length;
const currentActiveCount = mergedKeyEntities.filter(
({ status }) => status !== STATUS_REMOVED,
).length;

if (prevActiveCount > 0 && currentActiveCount === 0) {
onAllRemovedRef.current?.();
}

prevKeyEntitiesRef.current = mergedKeyEntities;
}, [mergedKeyEntities]);

// ZombieJ: Return the count of rest keys. It's safe to refactor if need more info.
const removeKey = React.useCallback((removedKey: React.Key) => {
setKeyEntities(prevKeyEntities => {
const nextRemovedKeyEntities = prevKeyEntities.map(entity => {
if (entity.key !== removedKey) return entity;
return {
keyEntities: nextKeyEntities,
...entity,
status: STATUS_REMOVED,
};
},
() => {
const { keyEntities } = this.state;
const restKeysCount = keyEntities.filter(
({ status }) => status !== STATUS_REMOVED,
).length;

if (restKeysCount === 0 && this.props.onAllRemoved) {
this.props.onAllRemoved();
}
},
);
};

render() {
const { keyEntities } = this.state;
const {
component,
children,
onVisibleChanged,
onAllRemoved,
...restProps
} = this.props;

const Component = component || React.Fragment;

const motionProps: CSSMotionProps = {};
MOTION_PROP_NAMES.forEach(prop => {
motionProps[prop] = restProps[prop];
delete restProps[prop];
});

return isSameKeyEntities(prevKeyEntities, nextRemovedKeyEntities)
? prevKeyEntities
: nextRemovedKeyEntities;
});
delete restProps.keys;

return (
<Component {...restProps}>
{keyEntities.map(({ status, ...eventProps }, index) => {
const visible = status === STATUS_ADD || status === STATUS_KEEP;
return (
<CSSMotion
{...motionProps}
key={eventProps.key}
visible={visible}
eventProps={eventProps}
onVisibleChanged={changedVisible => {
onVisibleChanged?.(changedVisible, { key: eventProps.key });

if (!changedVisible) {
this.removeKey(eventProps.key);
}
}}
>
{isRefNotConsumed(children)
? props =>
(children as ChildrenWithoutRef)({
...props,
index,
})
: (props, ref) => children({ ...props, index }, ref)}
</CSSMotion>
);
})}
</Component>
);
}
}
}, []);
Comment thread
QDyanbing marked this conversation as resolved.

const Component = component || React.Fragment;

const motionProps: CSSMotionProps = {};
MOTION_PROP_NAMES.forEach(prop => {
motionProps[prop] = restProps[prop];
delete restProps[prop];
});
delete restProps.keys;

return (
<Component {...restProps}>
{mergedKeyEntities.map(({ status, ...eventProps }, index) => {
const visible = status === STATUS_ADD || status === STATUS_KEEP;
return (
<CSSMotion
{...motionProps}
key={eventProps.key}
visible={visible}
eventProps={eventProps}
onVisibleChanged={changedVisible => {
onVisibleChanged?.(changedVisible, { key: eventProps.key });

if (!changedVisible) {
removeKey(eventProps.key);
}
}}
>
{isRefNotConsumed(children)
? motionChildrenProps =>
(children as ChildrenWithoutRef)({
...motionChildrenProps,
index,
})
: (motionChildrenProps, ref) =>
children({ ...motionChildrenProps, index }, ref)}
</CSSMotion>
);
})}
</Component>
);
};

CSSMotionList.displayName = 'CSSMotionList';

return CSSMotionList;
}
Expand Down
101 changes: 101 additions & 0 deletions tests/CSSMotionList.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -173,4 +173,105 @@ describe('CSSMotionList', () => {
'1',
);
});

it('should support children ref when component is false', () => {
const CSSMotionList = genCSSMotionList(false);

const { container } = render(
<CSSMotionList motionName="transition" component={false} keys={['a']}>
{({ key }, ref) => (
<div ref={ref as React.Ref<HTMLDivElement>} className="motion-box">
{key}
</div>
)}
</CSSMotionList>,
);

expect(container.children).toHaveLength(1);
expect(container.querySelector('.motion-box')).toHaveTextContent('a');
Comment on lines +177 to +191

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

这个用例还没有真正覆盖到 children(props, ref) 分支。

现在即使第二个参数是 undefined<div ref={ref as ...}> 也照样能渲染成功,所以这里的断言只能证明文本渲染正常,抓不住 ref 兼容分支的回归。

建议补一个显式断言
 it('should support children ref when component is false', () => {
   const CSSMotionList = genCSSMotionList(false);
+  let receivedRef: React.Ref<HTMLDivElement> | undefined;

   const { container } = render(
     <CSSMotionList motionName="transition" component={false} keys={['a']}>
       {({ key }, ref) => (
-        <div ref={ref as React.Ref<HTMLDivElement>} className="motion-box">
+        (() => {
+          receivedRef = ref as React.Ref<HTMLDivElement>;
+          return (
+            <div ref={receivedRef} className="motion-box">
+              {key}
+            </div>
+          );
+        })()
-          {key}
-        </div>
       )}
     </CSSMotionList>,
   );

   expect(container.children).toHaveLength(1);
   expect(container.querySelector('.motion-box')).toHaveTextContent('a');
+  expect(receivedRef).toBeTruthy();
 });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
it('should support children ref when component is false', () => {
const CSSMotionList = genCSSMotionList(false);
const { container } = render(
<CSSMotionList motionName="transition" component={false} keys={['a']}>
{({ key }, ref) => (
<div ref={ref as React.Ref<HTMLDivElement>} className="motion-box">
{key}
</div>
)}
</CSSMotionList>,
);
expect(container.children).toHaveLength(1);
expect(container.querySelector('.motion-box')).toHaveTextContent('a');
it('should support children ref when component is false', () => {
const CSSMotionList = genCSSMotionList(false);
let receivedRef: React.Ref<HTMLDivElement> | undefined;
const { container } = render(
<CSSMotionList motionName="transition" component={false} keys={['a']}>
{({ key }, ref) => (
(() => {
receivedRef = ref as React.Ref<HTMLDivElement>;
return (
<div ref={receivedRef} className="motion-box">
{key}
</div>
);
})()
)}
</CSSMotionList>,
);
expect(container.children).toHaveLength(1);
expect(container.querySelector('.motion-box')).toHaveTextContent('a');
expect(receivedRef).toBeTruthy();
});
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/CSSMotionList.spec.tsx` around lines 177 - 191, The CSSMotionList test
does not actually verify the children(props, ref) ref-compatibility path, since
rendering still passes even when ref is undefined. Update the CSSMotionList spec
around the component={false} case to assert the ref is explicitly provided to
the child render function and/or observed on the rendered element, using the
CSSMotionList render prop signature and the existing motion-box assertion to
cover the real branch.

});

it('should not require onAllRemoved', () => {
const CSSMotionList = genCSSMotionList(false);

const Demo = ({ keys }: { keys: string[] }) => (
<CSSMotionList motionName="transition" keys={keys}>
{({ key }) => <div className="motion-box">{key}</div>}
</CSSMotionList>
);

const { rerender } = render(<Demo keys={['a']} />);

act(() => {
jest.runAllTimers();
});

expect(() => {
rerender(<Demo keys={[]} />);
act(() => {
jest.runAllTimers();
});
}).not.toThrow();
});

it('should skip state update when removed key is already removed', () => {
const CSSMotion = React.forwardRef<any, any>(
({ children, eventProps, onVisibleChanged }, _ref) => (
<button
className="trigger"
type="button"
onClick={() => {
onVisibleChanged(false);
onVisibleChanged(false);
}}
>
{children(eventProps)}
</button>
),
);
const CSSMotionList = genCSSMotionList(false, CSSMotion);

const Demo = ({ keys }: { keys: string[] }) => (
<CSSMotionList motionName="transition" keys={keys}>
{({ key }) => <span className="motion-box">{key}</span>}
</CSSMotionList>
);

const { container, rerender } = render(<Demo keys={['a']} />);
rerender(<Demo keys={[]} />);
const trigger = container.querySelector('.trigger');
fireEvent.click(trigger);

expect(container.querySelector('.motion-box')).toBeFalsy();
});

it('should update event props when key object shape changes', () => {
const CSSMotionList = genCSSMotionList(false);
const hasOwn = Object.prototype.hasOwnProperty;

const Demo = ({ keys }: { keys: CSSMotionListProps['keys'] }) => (
<CSSMotionList motionName="transition" keys={keys}>
{props => (
<div
className="motion-box"
data-has-foo={hasOwn.call(props, 'foo')}
data-has-bar={hasOwn.call(props, 'bar')}
/>
)}
</CSSMotionList>
);

const { container, rerender } = render(
<Demo keys={[{ key: 'a', foo: undefined }]} />,
);
const getBox = () => container.querySelector<HTMLDivElement>('.motion-box');

expect(getBox()).toHaveAttribute('data-has-foo', 'true');
expect(getBox()).toHaveAttribute('data-has-bar', 'false');

rerender(<Demo keys={[{ key: 'a', bar: undefined }]} />);

expect(getBox()).toHaveAttribute('data-has-foo', 'false');
expect(getBox()).toHaveAttribute('data-has-bar', 'true');
});
});
Loading