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

Computed property for nodes and config #22

Open
Gurzhii opened this issue Oct 13, 2022 · 7 comments
Open

Computed property for nodes and config #22

Gurzhii opened this issue Oct 13, 2022 · 7 comments

Comments

@Gurzhii
Copy link

Gurzhii commented Oct 13, 2022

Hello! First of all, I want to thank you for the job you've done!

I have one question, I can't find in the docs. How can I use nodes and config as computed properties? It's not working for me at all. It renders the structure but select/expand features are not working at all.

Issue: I need to combine data from the parent with the initial tree state. (parent data is reactive and may be changed on the fly, so I need to use computed prop in the child component where I have the tree)

Example: https://codesandbox.io/s/basic-forked-h5h960?file=/src/App.vue

@roberto910907
Copy link

@Gurzhii, I found the same. It works if I use Vue 3 refs, but using computed properties(Pinia getters) is not working.

I think I will check the package's source code to see if I can find the issue and a workaround for it.

@roberto910907
Copy link

@N00ts here's the link to reproduce the issue, any suggestions?

@roberto910907
Copy link

@Gurzhii @N00ts, here's a quick update:

The tree will only work as expected if you pass a reactive property such as ref or reactive.

This is because the nodes are a computed property here:

    const state: IState = {
        id: uniqueId(),
        nodes: computed(() => nodes.value),
        config: computed(() => config.value),

From the docs:

A computed property automatically tracks its reactive dependencies.

Just an example so you can understand:

This is reactive:

  data() => {
    return {
      defaultNodes: {
         ....
      }
   },

   computed: {
    nodes() {
      return this.defaultNodes;
    },
  },

This is NOT reactive:

  computed: {
    nodes() {
      return {
        id1: {
          text: "text1",
          children: ["id11", "id12", "id13"],
        },
    } 

When you have computed properties not relying on reactive data, Vue returns a plain object instead of a Proxy.

Also, when using toRefs and destructuring the nodes and config props values, nodes.values and config.values will be reactive only if they're initially reactive values. Please check this example.

@N00ts That said, I think that a good solution could be checking if the refs are reactive already, if not, we can create a ref with the value and ensure that is reactive, something like this:

import { toRefs, computed, ComputedRef, Ref, ref, isReactive } from 'vue';

export function createState(props: ITreeProps): string {
    let { nodes, config } = toRefs(props);

    if(!isReactive(nodes.value)) {
        nodes = ref(nodes.value);
    }
    ....
}    

What do you think?

@roberto910907
Copy link

@N00ts If this looks good, I will be happy to open a pull request.

@N00ts
Copy link
Owner

N00ts commented Dec 17, 2022

Looks fine ! Or maybe juste error message to avoir people making this mistake !

@roberto910907
Copy link

@N00ts, there are many cases that we'll cover by adding these changes. For example, I'd say the very basic example you can show is passing the nodes and config as plain objects without declaring any variables(ref or reactive), and this is not currently working either, please check here.

@N00ts
Copy link
Owner

N00ts commented Dec 18, 2022

Yes but just a warning message can be a good thing instead of letting people having bad practices about Reactivity, don't you think ?

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

3 participants