Reapply #7089 and handle arrays of primitives/arrays #8408

timtrinidad · 2019-05-01T21:41:48Z

Summary

The first commit is the simple revert so the second commit outlines the changes from #7089.

The existing deepMergeArray assumed that the array elements were all objects.
This update checks specifically if the array element is primitive (if so, simply replace instead of calling deepMerge again)
It also checks if the array element is also an array, in which case it skips calling deepMerge (which would convert the array into an object) and instead calls deepMergeArray directly.

Test plan

scotthovestadt · 2019-05-01T21:44:09Z

Can you call out what changed? I expected (maybe wrong) to see an additional reference to $$typeof in there to avoid the expect.any(X) matchers.

(Of course, if the regression tests pass, happy to merge.)

timtrinidad · 2019-05-01T21:47:38Z

Ah yes - sorry about that! Description updated.. There is a new typeof in deepMergeArray that specifically checks for primitives.

packages/jest-snapshot/src/utils.ts

…gative ones

codecov-io · 2019-05-01T22:22:17Z

Codecov Report

Merging #8408 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #8408      +/-   ##
==========================================
+ Coverage   60.58%   60.61%   +0.03%     
==========================================
  Files         269      269              
  Lines       11052    11066      +14     
  Branches     2695     2699       +4     
==========================================
+ Hits         6696     6708      +12     
- Misses       3771     3772       +1     
- Partials      585      586       +1

Impacted Files	Coverage Δ
packages/jest-snapshot/src/utils.ts	`93.58% <100%> (+1.16%)`	⬆️
packages/jest-haste-map/src/crawlers/node.ts	`80.76% <0%> (-2.13%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 43d1cf6...9dbc98f. Read the comment docs.

packages/jest-snapshot/src/utils.ts

SimenB · 2019-06-11T20:47:50Z

Could you merge in/rebase on master just to verify CI is happy?

thymikee

Looks like CI is happy, I left 2 nits. I'll also repeat my comment that was not answered about reusing deepCyclicCopy: #7089 (comment). Is there anything preventing us from that?

thymikee · 2019-06-12T18:55:03Z

packages/jest-snapshot/src/utils.ts

@@ -178,13 +178,37 @@ export const saveSnapshotFile = (
  );
 };

+const deepMergeArray = (target: Array<any>, source: Array<any>) => {
+  // Clone target
+  const mergedOutput = target.slice();


Using Array.from(target) would be more obvious and wouldn't require a comment

thymikee · 2019-06-12T18:57:14Z

packages/jest-snapshot/src/utils.ts

+      // Target is an array
+      mergedOutput[index] = deepMergeArray(target[index], sourceElement);
+    } else if (isObject(targetElement)) {
+      // Target is a (non-array) object - recursively merge


I don't think we need these comments (line 192 & 188), can you remove these?

timtrinidad · 2019-06-13T15:19:48Z

@thymikee I was under the impression that your comment referred to the original method:

// Convert arrays to objects, merge, convert back to array

It does not do that anymore - can you outline what is expensive with the current implementation?

thymikee · 2019-06-13T15:27:27Z

I'm not concerned about perf in current implementation, sorry if that wasn't clear. I only want to know if we can reuse deepCyclicCopy which handles arrays. And if not, what's preventing us from using it?

timtrinidad · 2019-06-13T16:41:19Z

Sorry if I'm being dense, but I'm still not sure I understand. Do you mean using deepCyclicCopy instead of Array.from() or instead of deepMerge?

thymikee · 2019-06-13T16:50:48Z

Instead of deepMerge altogether

timtrinidad · 2019-06-13T18:57:36Z

I might be misunderstanding deepCyclicCopy. From the tests, it looks like it only copies a single array/object and does not take two arrays/objects as arguments. This is different than deepMerge, which takes two different inputs and combines them into one.

thymikee · 2019-06-13T19:35:58Z

Ok that's fine. @SimenB feel free to merge it :)

SimenB · 2019-06-13T21:23:32Z

Thanks for being patient with us, @timtrinidad!

timtrinidad · 2019-06-13T21:25:19Z

No problem - happy to help!

github-actions · 2021-05-11T18:13:08Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

timtrinidad added 2 commits May 1, 2019 17:15

Reapply jestjs#7089

1a3b280

handle simple arrays and arrays of arrays

af7261b

timtrinidad mentioned this pull request May 1, 2019

Handle arrays when merging snapshots #7089

Merged

facebook-github-bot added the cla signed label May 1, 2019

jeysal reviewed May 1, 2019

View reviewed changes

packages/jest-snapshot/src/utils.ts Outdated Show resolved Hide resolved

remove unecessary check and reorder for positive checks instead of ne…

c9d1aff

…gative ones

SimenB mentioned this pull request May 2, 2019

Revert #7089 and add tests to catch the regression(s) in the future. #8405

Merged

jeysal reviewed May 2, 2019

View reviewed changes

packages/jest-snapshot/src/utils.ts Outdated Show resolved Hide resolved

trivikr mentioned this pull request May 3, 2019

suggestion: Use Codecov for showing coverage information aws/aws-sdk-js-v3#240

Closed

use isObject

4e29634

timtrinidad force-pushed the master branch from ba37ba8 to 4e29634 Compare June 11, 2019 19:23

Merge branch 'master' of https://github.com/facebook/jest

a65fd9e

thymikee reviewed Jun 12, 2019

View reviewed changes

PR comments - remove frivolous comments, use Array.from(target)

9dbc98f

timtrinidad force-pushed the master branch from dac812e to 9dbc98f Compare June 13, 2019 16:27

SimenB merged commit 62ca17c into jestjs:master Jun 13, 2019

github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reapply #7089 and handle arrays of primitives/arrays #8408

Reapply #7089 and handle arrays of primitives/arrays #8408

timtrinidad commented May 1, 2019 •

edited

scotthovestadt commented May 1, 2019

timtrinidad commented May 1, 2019 •

edited

codecov-io commented May 1, 2019 •

edited

SimenB commented Jun 11, 2019

thymikee left a comment

thymikee Jun 12, 2019

thymikee Jun 12, 2019

timtrinidad commented Jun 13, 2019

thymikee commented Jun 13, 2019

timtrinidad commented Jun 13, 2019

thymikee commented Jun 13, 2019

timtrinidad commented Jun 13, 2019 •

edited

thymikee commented Jun 13, 2019

SimenB commented Jun 13, 2019

timtrinidad commented Jun 13, 2019

github-actions bot commented May 11, 2021

Reapply #7089 and handle arrays of primitives/arrays #8408

Reapply #7089 and handle arrays of primitives/arrays #8408

Conversation

timtrinidad commented May 1, 2019 • edited

Summary

Test plan

scotthovestadt commented May 1, 2019

timtrinidad commented May 1, 2019 • edited

codecov-io commented May 1, 2019 • edited

Codecov Report

SimenB commented Jun 11, 2019

thymikee left a comment

Choose a reason for hiding this comment

thymikee Jun 12, 2019

Choose a reason for hiding this comment

thymikee Jun 12, 2019

Choose a reason for hiding this comment

timtrinidad commented Jun 13, 2019

thymikee commented Jun 13, 2019

timtrinidad commented Jun 13, 2019

thymikee commented Jun 13, 2019

timtrinidad commented Jun 13, 2019 • edited

thymikee commented Jun 13, 2019

SimenB commented Jun 13, 2019

timtrinidad commented Jun 13, 2019

github-actions bot commented May 11, 2021

timtrinidad commented May 1, 2019 •

edited

timtrinidad commented May 1, 2019 •

edited

codecov-io commented May 1, 2019 •

edited

timtrinidad commented Jun 13, 2019 •

edited