Allow arbitrary number of prop objects to be passed to mergeProps #776
Conversation
brookslybrand
requested review from
dannify,
devongovett and
snowystinger
as code owners
|
Wow, awesome! Thanks for tackling this. |
| res[key] = classNames(a.UNSAFE_className, b.UNSAFE_className); | ||
| export function mergeProps<T extends Props, U extends Props[]>( | ||
| a: T, | ||
| ...rest: U |
There was a problem hiding this comment.
Does this mean all of the subsequent props objects after the first one must be the same type, or does it allow differences as long as they are all compatible with Props? If so, do we still need to treat a separately or would it make sense to make them all part of the same rest parameter?
There was a problem hiding this comment.
They can all be different. I probably should have added a test that showed that, but here's sandbox I have up with an example: https://codesandbox.io/s/mergeprops-irpn6?file=/src/index.ts
At first I did just spread all of the args, but then I separated a out just to start off the result variable a little more easily, but I suppose that's not actually super necessary. Maybe it would be better to switch it to just being ...rest or ...props or something.
There was a problem hiding this comment.
Alright, I went ahead and got rid of a and just have ...args passed in. I also added another test to show that it will allow you to add other props in the later objects and it will merge them altogether.
| let c = {onClick: () => console.log(message3)}; | ||
| let mergedProps = mergeProps(a, b, c); | ||
| mergedProps.onClick(); | ||
| expect(console.log).toHaveBeenCalledWith(message1); |
There was a problem hiding this comment.
Would you mind verifying the order they are called in too? We don't want to accidentally change that on people
There was a problem hiding this comment.
@snowystinger changed to toHaveBeenNthCalledWith
| it('combines classNames', function () { | ||
| let className1 = 'primary'; | ||
| let className2 = 'hover'; | ||
| let className3 = 'hover'; |
There was a problem hiding this comment.
this raises an interesting point, should we be deduping classnames?
there's nothing particularly wrong with multiple of the same that I'm aware of, so I'm inclined to leave it alone
edit: quick search of className, i don't think it does it for us
There was a problem hiding this comment.
Ah, that's funny, this was actually a mistake in my test, but you're right, I think just letting the classnames library do what it does is probably the best move.
| }); | ||
|
|
||
| it('combines callbacks', function () { | ||
| console.log = jest.fn(); |
There was a problem hiding this comment.
bit of a weird question, this is a global we're assigning a mock function to. I have always cleaned that up in an afterEach, but that may just be me being careful. Do you know if this will get cleaned up after this it?
There was a problem hiding this comment.
actually, lets not do this with console.log at all, lets just make a function we can call. That way we don't obscure console messages during this suite unnecessarily
There was a problem hiding this comment.
Oh yeah, that makes a lot of sense. Alright, cleaned up all of those
| console.log = jest.fn(); | ||
| let message1 = 'click1'; | ||
| let message2 = 'click2'; | ||
| let message3 = 'click3'; | ||
| let mergedProps = mergeProps( | ||
| {onClick: () => console.log(message1)}, | ||
| {onClick: () => console.log(message2)}, | ||
| {onClick: () => console.log(message3)} | ||
| ); | ||
| mergedProps.onClick(); | ||
| expect(console.log).toHaveBeenNthCalledWith(1, message1); | ||
| expect(console.log).toHaveBeenNthCalledWith(2, message2); | ||
| expect(console.log).toHaveBeenNthCalledWith(3, message3); | ||
| expect(console.log).toHaveBeenCalledTimes(3); |
There was a problem hiding this comment.
let mockFn = jest.fn();
let message1 = 'click1';
let message2 = 'click2';
let message3 = 'click3';
let mergedProps = mergeProps(
{onClick: () => mockFn(message1)},
{onClick: () => mockFn(message2)},
{onClick: () => mockFn(message3)}
);
mergedProps.onClick();
expect(mockFn).toHaveBeenNthCalledWith(1, message1);
expect(mockFn).toHaveBeenNthCalledWith(2, message2);
expect(mockFn).toHaveBeenNthCalledWith(3, message3);
expect(mockFn).toHaveBeenCalledTimes(3);
| }); | ||
|
|
||
| it('merges props with different keys', function () { | ||
| console.log = jest.fn(); |
There was a problem hiding this comment.
same thing with this test, lets not overwrite console.log
|
Thank you! 😍 |
Currently
mergePropsonly allows 2 arguments to be passed in at a time, which results in extra nesting whenever there are more than 2 prop objects that need to be merged, as seen in this example.So instead of:
You can just do:
@devongovett mentioned on twitter that the team ran into issues with TypeScript handling an arbitrary number.
I was able to find a solution on stackoverflow that helped me solve this, which I cited in the source code.
Please let me know if any more testing needs to be done or if there are any other steps I should take on this PR.