Fix mergeCollection bug #66

tgolen · 2021-04-22T22:01:06Z

Details

This fixes a bug found in mergeCollection

Related Issues

https://github.com/Expensify/Expensify/issues/161384

Automated Tests

There is a test added which properly tests the behavior

Linked PRs

Expensify/App#2543

github-actions · 2021-04-22T22:01:23Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

tgolen · 2021-04-22T22:12:18Z

I have read the CLA Document and I hereby sign the CLA

marcaaron

Looks great and nice catch!

marcaaron · 2021-04-22T22:15:20Z

lib/Onyx.js

+                    // If `newData` happens to not exist, then set the state back to the previous value because
+                    // if the state is set to an undefined value, then the item reverts to whatever is in
+                    // defaultProps
+                    [subscriber.statePropertyName]: typeof newData !== 'undefined' ? newData : prevState[subscriber.statePropertyName],


Should we use _.isUndefined() ?

Ah, yeah... that's a good call. I guess we do have underscore here.

tgolen · 2021-04-22T22:20:14Z

Updated to use underscore

marcaaron · 2021-04-22T22:34:00Z

lib/Onyx.js

+                    // defaultProps
+                    [subscriber.statePropertyName]: !_.isUndefined(newData)
+                        ? newData
+                        : prevState[subscriber.statePropertyName],


Thinking more on this... this could maybe be filed under "unrelated" or "not a problem", but I find it interesting (or rather incorrect/inefficient) that if a subscriber is connected to a single key, but it's data hasn't changed at all ... that we are still calling setState() with previous data?

That's not super great because it means that we will still trigger re-renders on those connected components and for no reason at all. So, probably if we have undefined then we should do less than set the previous state again and should do... well... nothing?

Yeah, I think you make a good point. While it's not a problem currently, it's not a great design either. The reason I put it inside setState is because that's where it is getting newData. But now that I look at it closely, newData should be able to be retrieved before calling setState() and in the case where it is undefined, we can just skip calling setState(). How's that sound?

Yep, sounds good to me. 👍

marcaaron · 2021-04-22T22:39:21Z

tests/unit/withOnyxTest.js

+                expect(onRender2.mock.calls[0][0].testObject).toStrictEqual({ID: 2});
+                expect(onRender2.mock.calls[1][0].testObject).toStrictEqual({ID: 2});
+                expect(onRender3.mock.calls[0][0].testObject).toStrictEqual({ID: 3});
+                expect(onRender3.mock.calls[1][0].testObject).toStrictEqual({ID: 3});


Like my expectation would be... not that this gets called again with same data but not called at all ? Why should it re-render since nothing has changed?

tgolen · 2021-04-22T23:14:23Z

OK, updated. Now with less re-renderings!

chiragsalian

LGTM and i like the changes 👍

tgolen · 2021-04-23T16:26:38Z

OK, before merging this, I want to give this a little more testing because I haven't been able to test the most recent changes yet. Once I do that testing, I'll let you know when this has a green-light to merge.

deetergp · 2021-04-23T17:36:06Z

tests/unit/withOnyxTest.js

+        const onRender2 = jest.fn();
+        const onRender3 = jest.fn();
+
+        // GIVEN there is a collection with three simple items in it


NAB: I REALLY appreciate the GIVEN/WHEN/THEN docs here! It makes it so much easier to follow what's going on and what we're testing for. :bigfan:

Yeah, I love that format for testing comments. @nkuoch was the one that originally showed it to me!

deetergp

Looks good to me

tgolen · 2021-04-23T22:02:49Z

OK, I think I found another bug in my testing, but I want to write a test case to confirm that theory. Will continue this on Monday.

tgolen · 2021-04-26T16:41:03Z

All right, I am taking off the hold here. When I tested this on Friday I found another bug with how the new collection data was being merged into the state. I have added a test case to cover that, fixed the bug (using spread operators instead of lodash.merge()), and I've tested this in the IOU PR to ensure it works the way it's supposed to.

chiragsalian

LGTM

marcaaron · 2021-04-26T17:11:10Z

lib/Onyx.js

+            subscriber.withOnyxInstance.setState(prevState => ({
+                [subscriber.statePropertyName]: _.isObject(dataFromCollection)
+                    ? {
+                        ...prevState[subscriber.statePropertyName],


This seems potentially dangerous as it assumes we will always have at least an empty object for prevState[subscriber.statePropertyName] if we are dealing with object shaped data. If we try to spread something like undefined or null then we get 💥.

Maybe some combination of lodash.merge() could still be helpful here to help guard against that?

Maybe there's no problem with that actually. I could just have made that up in my head.

I think spreading undefined in objects works OK but arrays doesn't.

I guess the small difference between this and using lodashMerge() is that the spread is not going to recursively merge the data. So depending on how complicated that data is we could get some weird results. But we can address that if it becomes a problem.

Cool, thanks Marc. I think it'll be OK for now. Seeing as how everyone has approved this, I'm going to go ahead and merge it.

tgolen added 5 commits April 22, 2021 14:46

Adding a test for a bug with merge collection

53d964e

Clean up the test a little bit

141bda7

Fix for the bug

fed0198

Improve the check to only look for undefined values

c56b745

Fix the PR template

cb8e408

tgolen requested review from chiragsalian, marcaaron and a team April 22, 2021 22:01

tgolen self-assigned this Apr 22, 2021

MelvinBot requested review from stitesExpensify and removed request for a team April 22, 2021 22:01

tgolen mentioned this pull request Apr 22, 2021

Fix the IOU badge on newly created reports Expensify/App#2543

Merged

7 tasks

Lint

45b6bda

tgolen requested a review from a team as a code owner April 22, 2021 22:15

MelvinBot requested review from deetergp and removed request for a team April 22, 2021 22:15

marcaaron reviewed Apr 22, 2021

View reviewed changes

Use underscore is undefined

4aecf38

marcaaron reviewed Apr 22, 2021

View reviewed changes

Prevent multiple renderings

38d484b

Lint

8870f26

marcaaron previously approved these changes Apr 22, 2021

View reviewed changes

chiragsalian previously approved these changes Apr 23, 2021

View reviewed changes

tgolen changed the title ~~Fix mergeCollection bug~~ [HOLD testing] Fix mergeCollection bug Apr 23, 2021

stitesExpensify previously approved these changes Apr 23, 2021

View reviewed changes

deetergp reviewed Apr 23, 2021

View reviewed changes

deetergp previously approved these changes Apr 23, 2021

View reviewed changes

tgolen added 3 commits April 26, 2021 10:24

Remove all the unnecessary describes

b2ef2d5

Add a test that confirms there is a bug with mergeCollection

3397189

Add a fix for properly merging new data into the state

a8ab34e

tgolen dismissed stale reviews from deetergp, stitesExpensify, chiragsalian, and marcaaron via a8ab34e April 26, 2021 16:34

tgolen changed the title ~~[HOLD testing] Fix mergeCollection bug~~ Fix mergeCollection bug Apr 26, 2021

chiragsalian approved these changes Apr 26, 2021

View reviewed changes

marcaaron reviewed Apr 26, 2021

View reviewed changes

deetergp approved these changes Apr 26, 2021

View reviewed changes

marcaaron approved these changes Apr 26, 2021

View reviewed changes

tgolen merged commit accabbd into master Apr 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix mergeCollection bug #66

Fix mergeCollection bug #66

tgolen commented Apr 22, 2021

github-actions bot commented Apr 22, 2021 •

edited

Loading

tgolen commented Apr 22, 2021

marcaaron left a comment

marcaaron Apr 22, 2021

tgolen Apr 22, 2021

tgolen commented Apr 22, 2021

marcaaron Apr 22, 2021

tgolen Apr 22, 2021

marcaaron Apr 22, 2021

marcaaron Apr 22, 2021

tgolen commented Apr 22, 2021

chiragsalian left a comment

tgolen commented Apr 23, 2021

deetergp Apr 23, 2021

tgolen Apr 23, 2021

deetergp left a comment

tgolen commented Apr 23, 2021

tgolen commented Apr 26, 2021

chiragsalian left a comment

marcaaron Apr 26, 2021

marcaaron Apr 26, 2021

marcaaron Apr 26, 2021

marcaaron Apr 26, 2021

tgolen Apr 26, 2021

Fix mergeCollection bug #66

Fix mergeCollection bug #66

Conversation

tgolen commented Apr 22, 2021

Details

Related Issues

Automated Tests

Linked PRs

github-actions bot commented Apr 22, 2021 • edited Loading

tgolen commented Apr 22, 2021

marcaaron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgolen commented Apr 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgolen commented Apr 22, 2021

chiragsalian left a comment

Choose a reason for hiding this comment

tgolen commented Apr 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deetergp left a comment

Choose a reason for hiding this comment

tgolen commented Apr 23, 2021

tgolen commented Apr 26, 2021

chiragsalian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 22, 2021 •

edited

Loading