Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from
Jun 13, 2019

Conversation

timtrinidad
Copy link
Contributor

@timtrinidad timtrinidad commented May 1, 2019

Summary

See #7089 (comment)

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
Copy link
Contributor

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
Copy link
Contributor Author

timtrinidad commented May 1, 2019

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

@codecov-io
Copy link

codecov-io commented May 1, 2019

Codecov Report

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

Impacted file tree graph

@@            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.

@SimenB
Copy link
Member

SimenB commented Jun 11, 2019

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

Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

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?

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

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

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

@timtrinidad
Copy link
Contributor Author

@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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Collaborator

Instead of deepMerge altogether

@timtrinidad
Copy link
Contributor Author

timtrinidad commented Jun 13, 2019

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
Copy link
Collaborator

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

@SimenB SimenB merged commit 62ca17c into jestjs:master Jun 13, 2019
@SimenB
Copy link
Member

SimenB commented Jun 13, 2019

Thanks for being patient with us, @timtrinidad!

@timtrinidad
Copy link
Contributor Author

No problem - happy to help!

@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants