feat: setData #52
feat: setData #52
Conversation
| setData(data: Record<string, any>) { | ||
| // lodash.merge merges by *reference* so this will update | ||
| // any existing data with the newly passed data. | ||
| merge(this.componentVM.$data, data) |
There was a problem hiding this comment.
Should we guard against setting data that is not in $data already? Not sure
There was a problem hiding this comment.
Let's check how Vue 3 behaves. I wonder if the new reactivity API allows you to update with new properties? I think we should match whatever Vue 3 does.
|
Should I re-base and merge this? I forgot why we left this open. |
I think we said to wait until later alphas, and see if people asked about this particular feature. I won't be using it personally, but some concerns were raised in the RFC so we might just add it and keep it as a low priority feature. |
A test that uses `setData` is skipped. `setData` is not yet implemented See: vuejs/test-utils/pull/52
|
Dropped lodash dep, will need to write our own |
|
I will close this for now, we can implement if/when we need it. |
|
We at BootstrapVue are using I'am also willing to test this with our current tests and give feedback. |
|
Would you like help in refactoring to move away from setData? I see 11 code
usages in bootstrap Vue (
https://github.com/bootstrap-vue/bootstrap-vue/search?q=setData&type=
) and I feel comfortable submitting a PR to migrate off of it.
Implementing setData allows users to test the implementation of the
component instead of the contract and is generally a bad practice (which is
why Adria, Lachlan, and I don’t use it or promote it ever)
…On Thu, Oct 22, 2020 at 5:13 PM Jacob Müller ***@***.***> wrote:
We at BootstrapVue <https://github.com/bootstrap-vue/bootstrap-vue> are
using wrapper.setData() quite heavily.
We are currently in the process of creating a Vue 3 compatible version and
it would be good to have that feature in VTU v2.
I'am also willing to test this with our current tests and give feedback.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#52 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAVL4BA37EIDDG6MIDTH3NDSMCN75ANCNFSM4MF4FQTA>
.
|
|
You are right, we don't use it that much. |
|
I think we do need to implement this - backwards compat is pretty important, we learned this from some of the aggressive deprecations in 1.0. I think this feature is very easy to implement. I also expect a lot of push-back if we were to deprecate this. I would recommend implementing it and referencing it in the API reference part of the docs, but not including it in any of our guides (I think we settled on this been unideal in most cases - just use the What does everyone think? |
|
@lmiller1990 I would prefer to have this in v2 too. A guide in the docs on why you should avoid using it and what better alternatives are seems to be a good idea. |
|
The reason it's not ideal is because arbitrarily setting data on a component is not something a user can do - ideally, test like a user does. Eg, if the data is set via a button, click the button with Either way, we should support this. I will make a PR in the near future unless someone else does first - you can reference this PR to see how it can be implemented. |
|
Surprised to hear your stance on this -- I remember this being an unknown
amount of complexity. I'm happy to give users a non-breaking path. +1
…On Thu, Oct 22, 2020 at 7:46 PM Lachlan Miller ***@***.***> wrote:
The reason it's not ideal is because arbitrarily setting data on a
component is not something a user can do - ideally, test like a user does.
Eg, if the data is set via a button, click the button with trigger etc.
Either way, we should support this. I will make a PR in the near future
unless someone else does first - you can reference this PR to see how it
can be implemented.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#52 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAVL4BCE22BVWC6QJGIVZW3SMC74FANCNFSM4MF4FQTA>
.
|
|
In terms of implementation, this was working https://github.com/vuejs/vue-test-utils-next/pull/52/files (this PR). Unless I am missing some specific use case. Don't get me wrong, I still think I also think after 1.0 which was met with some criticism (many deprecations) we should learn from that experience and avoid breaking changes/surprises, if at all possible. If there is a technical limitation, that's fine, but if not let's do it. |
|
I'm also OK with adding |
5 participants
Still not a big fan of this feature, but at least it was easy to implement 👍