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

How to use with props? #16

Open
pmorch opened this issue May 4, 2023 · 8 comments
Open

How to use with props? #16

pmorch opened this issue May 4, 2023 · 8 comments

Comments

@pmorch
Copy link

pmorch commented May 4, 2023

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

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

pmorch commented May 5, 2023

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 pmorch closed this as completed May 5, 2023
@pmorch pmorch reopened this May 5, 2023
@pmorch
Copy link
Author

pmorch commented May 5, 2023

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

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

pmorch commented May 9, 2023

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

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

pmorch commented May 11, 2023

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

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants