Alfred-Skyblue / vue-draggable-plus

Universal Drag-and-Drop Component Supporting both Vue 3 and Vue 2
https://vue-draggable-plus.pages.dev/en/
MIT License
2.74k stars 105 forks source link

How to use with props? #16

Open pmorch opened 1 year ago

pmorch commented 1 year ago

In my use case, my component gets the array from a parent component in a prop. Isn't this likely more common in real-world usage than having a data array? What is the best way to use <VueDraggable> with a prop? This might be a something to document or an example to show...

All the examples use v-model, but when I tried that, I get this error:

[plugin:vite:vue] v-model cannot be used on a prop, because local prop bindings are not writable. Use a v-bind binding combined with a v-on listener that emits update:x event instead.

Which does make sense. If I try to use v-bind instead, I get:

caught (in promise) DOMException: Failed to execute 'setAttribute' on 'Element': '0' is not a valid attribute name.
    at patchAttr (http://localhost:5173/node_modules/.vite/deps/chunk-3NMN3MUW.js?v=08779242:8103:10)
    at patchProp (http://localhost:5173/node_modules/.vite/deps/chunk-3NMN3MUW.js?v=08779242:8234:5)
    at mountElement (http://localhost:5173/node_modules/.vite/deps/chunk-3NMN3MUW.js?v=08779242:5786:11)
    at processElement (http://localhost:5173/node_modules/.vite/deps/chunk-3NMN3MUW.js?v=08779242:5764:7)
    at patch (http://localhost:5173/node_modules/.vite/deps/chunk-3NMN3MUW.js?v=08779242:5698:11)
    at ReactiveEffect.componentUpdateFn [as fn] (http://localhost:5173/node_modules/.vite/deps/chunk-3NMN3MUW.js?v=08779242:6128:11)
    at ReactiveEffect.run (http://localhost:5173/node_modules/.vite/deps/chunk-3NMN3MUW.js?v=08779242:405:19)
    at instance.update (http://localhost:5173/node_modules/.vite/deps/chunk-3NMN3MUW.js?v=08779242:6220:52)
    at setupRenderEffect (http://localhost:5173/node_modules/.vite/deps/chunk-3NMN3MUW.js?v=08779242:6228:5)
    at mountComponent (http://localhost:5173/node_modules/.vite/deps/chunk-3NMN3MUW.js?v=08779242:6046:5)

Which is less straight-forward to interpret.

What I've ended up doing is to have a copy of the array prop and then have watchers on both the prop and the copy to update "the other one". But it feels like a hack...

Alfred-Skyblue commented 1 year ago

In fact, it is possible to modify the prop for the object type, but the point of the prop is not modified inside the component, so update:xxx will not be triggered. In Vue Draggable, only the order of the list is modified. If you simply pass the modelValue as prop, It is also possible to achieve two-way binding.

pmorch commented 1 year ago

I see. If list is a prop, then the prop itself does not get modified, but the order of elements in the list array do.

<template>
    <VueDraggable
      :modelValue="list"
      ...
    >
      ...
    </VueDraggable>
</template>

<script>
...
export default {
  props: ['list'],
  ...
};
</script>

I'm often confused about whether modifying deep property values is ok / best practice in Vue. But this works for vue-draggable-plus. Gotcha.

Full working example here: https://stackblitz.com/edit/github-b2xatc-a1fgqc?file=src%2FApp.vue,src%2FDraggable.vue

pmorch commented 1 year ago

Hi again,

I've been reading up a little on Vue. And the guide explicitly calls it out as best-practice for a component not to modify parent arrays and objects:

Mutating Object / Array Props

When objects and arrays are passed as props, while the child component cannot mutate the prop binding, it will be able to mutate the object or array's nested properties. This is because in JavaScript objects and arrays are passed by reference, and it is unreasonably expensive for Vue to prevent such mutations.

The main drawback of such mutations is that it allows the child component to affect parent state in a way that isn't obvious to the parent component, potentially making it more difficult to reason about the data flow in the future. As a best practice, you should avoid such mutations unless the parent and child are tightly coupled by design. In most cases, the child should emit an event to let the parent perform the mutation.

So vue-draggable-plus achieves its prop behavior by modifying the contents of the provided prop array, which is not best practice.

I understand that this perhaps is unreasonable to fix, or you may disagree with Vue's official opinion on modifying props, and I'll be fine if you just close this, but I'll let you close it. Perhaps this is something to note in the README.md at least.

But I'm fine either way. I understand now.

Alfred-Skyblue commented 1 year ago

I know that this behavior breaks Vue's single-item data flow. Of course, there are other ways to achieve it, or I can deep copy a value and use a watch to monitor its changes and notify the user of updates, but I think it will be more troublesome for users to use this way. So I chose this method. I don't think Vue's official design is unreasonable, but I think that no matter which design method is used, it is for the convenience of users.

pmorch commented 1 year ago

In fact you wouldn't need to do a deep copy or a deep watch - a shallow copy and watch would be enough since you only ever change the top-level array.

I can say that for me it was very confusing because I was expecting that vue-draggable-plus would not modify my prop directly, which is why I also filed #15 while trying to listen for updates. Had this non-best-practice (and therefore at least to me surprising) behaviour been documented, it would have saved me some time.

I agree that in the end my code is simpler when vue-draggable-plus just modifies the prop.

Alfred-Skyblue commented 1 year ago

This is not the cause of #15. The problem is that I intercept the onUpdate method, because I need to intercept it and update it internally. In fact, I call the update passed by the user first, and then update the element location, that's what's causing the #15, I don't know what kind of confusion the current problem has caused you? Or what kind of problems do you encounter in use? Or do you simply think it's unreasonable for me to break one-way data flow?

pmorch commented 1 year ago

I totally understand that #15 had a different cause (also evidenced by the fix in 33a73ca64f26)

But the reason I discovered #15 was because I expected that I had to use @update="listUpdate" to listen for updates because I was not expecting a component to change my props "behind my back". That led me to discover #15, where I used nextTick() as a workaround and filed #15.

Alfred-Skyblue commented 1 year ago

If you need to monitor updates, you can use watch to monitor it. If you need to notify the outside when updating internally, then do the same. update is provided by Sortable, it is only used for view update, it does not mean when it is executed, data will be updated.