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

Bug: setData with classes/Object prototypes does not work correctly #1851

Closed
jacksongross opened this issue Nov 9, 2022 · 5 comments · Fixed by #2166
Closed

Bug: setData with classes/Object prototypes does not work correctly #1851

jacksongross opened this issue Nov 9, 2022 · 5 comments · Fixed by #2166
Labels
bug Something isn't working

Comments

@jacksongross
Copy link

Describe the bug
When testing a component that calls setData with a non-primitive class/object, the object gets set into the component VM without the prototype properties which breaks what should be a straightforward test. If you set the data property on the VM property directly with wrapper.vm.foo = bar it works as expected.

To Reproduce
Made a very basic example that illustrates the bug relating to the mergeDeep behaviour when calling setData
https://stackblitz.com/edit/vue3-script-setup-with-vite-prcxxz?file=src%2Fcomponents%2FHelloWorld.spec.ts,src%2Fcomponents%2FMyObject.js,src%2Fcomponents%2FHelloWorld.vue,src%2FApp.vue

Expected behavior
I expect that calling setData will simply set the property on the component's VM without modification.

Related information:

  • @vue/test-utils version: 2.0.0
  • Vue version: 3.2.37
  • node version: 16.15.0
  • npm (or yarn) version: 8.10.0

I can see a fix was applied specifically for Date objects but feel the real problem isn't solved.

@jacksongross jacksongross added the bug Something isn't working label Nov 9, 2022
@cexbrayat
Copy link
Member

Hi @jacksongross

Thanks for the repo! This indeed looks like an issue.

deepMerge probably needs to handle this edge case. Our codebase mentions a SO post that was the source of the algorithm https://stackoverflow.com/questions/27936772/how-to-deep-merge-instead-of-shallow-merge/48218209#48218209

It looks like the answer was updated with a reference to https://jhildenbiddle.github.io/mergician/#/ I wonder if this lib handles this use-case? Maybe we could use it, or at least improve mergeDeep with their findings.

Would you like to try and open a PR to solve it? Any help would be gladly appreciated!

@jacksongross
Copy link
Author

Yeah I might take a stab at this when I have some extra time in the next week or so.

@lmiller1990
Copy link
Member

Just to check you said

@vue/test-utils version: 2.0.0

We are on 2.2.2 now - there has been many releases since 2.0.0, might be worth updating? That said, I don't think we've changed deepMerge since then.

@jacksongross
Copy link
Author

Just to check you said

@vue/test-utils version: 2.0.0

We are on 2.2.2 now - there has been many releases since 2.0.0, might be worth updating? That said, I don't think we've changed deepMerge since then.

It makes no difference -- as you said, the deepMerge hasn't been updated. See https://stackblitz.com/edit/vue3-script-setup-with-vite-prcxxz?file=package.json

@lmiller1990
Copy link
Member

Right, fair enough. We should update deepMerge.

cexbrayat pushed a commit that referenced this issue Aug 22, 2023
Currently, when calling setData on a wrapper it will not retain methods on any constructed objects passed in. These methods reside in the prototype which the current logic does not persist across. These changes will ensure that any prototype properties/methods are copied over to the proxied data object.

fixes #1851
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants