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

Meteor properties over reactive? #49

Open
vbgm opened this issue Dec 23, 2018 · 3 comments · May be fixed by #60 or #61
Open

Meteor properties over reactive? #49

vbgm opened this issue Dec 23, 2018 · 3 comments · May be fixed by #60 or #61

Comments

@vbgm
Copy link

vbgm commented Dec 23, 2018

<template>

  <div v-if='test1'/>

</template><script>

  export default {
    computed: {
      test1 () {
        console.log('test1')
      }
    },
    meteor: {
      $lazy: true,
      test2 () {
        console.log('test2')
      }
    }
  }

</script>

In the above example:

  1. 'test1' is triggered once - OK
  2. 'test2' is triggered once when '$lazy' is true and twice when '$lazy' is false - NOT OK

Expected behaviour:
The 'test2' should not trigger at all when '$lazy' is enabled, or be triggered only once when $lazy is disabled.

Notes:
'Vue.config.meteor.freeze = true' does not fix it either.

@vbgm vbgm changed the title Meteor property over reactive? Meteor properties over reactive? Dec 23, 2018
@wildhart
Copy link

wildhart commented Jan 9, 2019

Same here, the meteor.property is being run at least once when the page is rendered regardless of whether it is used in the template or not, even with $lazy='true'.

What's worse, it can result in unexpected errors if the meteor.property relies on props or data which might be invalid in that situation. e.g:

<template>
    <div>
        <div v-if="user">{{user.profile.name}}</div>
        <div v-else>
            all users:
            <ul><li v-for="u in allUsers" :key="_id">{{u.profile.name}}</li></ul>
        </div>
    </div>
</template>

<script>
export default {
    props: ['user', 'query'], // we only need one of these
    meteor: {
        allUsers() {
            return Meteor.users.find(this.query, {fields: {"profile.name"}: 1);
        }
    }
}
</script>

(this is a contrived example, but I have some real world experience of this bug in more complicated components.)

Here the allUsers property isn't required if a user object is passed a prop, in which case the component might reasonably be called without the query prop at all. This would cause this.query to be undefined in allUsers(), even though allUsers isn't necessary.

My work-around is to do this instead:

export default {
    props: ['user', 'query'], // we only need one of these
    computed: {
        allUsers() { return this.$autorun(() => {
            if (!this.query) console.error('component called without any valid props');
            return this.query ? Meteor.users.find(this.query, {fields: {"profile.name"}: 1}) || [];
        })}
    }
}

Now the allUsers property isn't called unless it is actually needed (in which case the parent component should hopefully have passed the query prop instead.

@vbgm
Copy link
Author

vbgm commented Aug 5, 2019

@Akryum are there any chances you could pay attention to issues with Meteor integration? To me (and hopefully to many), native Meteor (with Tracker and MiniMongo) has more advantages than Apollo.

@red-meadow
Copy link

red-meadow commented Nov 1, 2019

@vblagomir @wildhart

Hello, guys! I decided to look under the hood and try to fix this. It turned out that we have two separated issues here: when $lazy is true and when it isn't.

The behavior of $lazy may not be a bug at all: @Akryum added it in a single commit and I guess the idea is that $lazy disables reactivity but still provides initial values.

I created two PRs: #60 and #61. I'm not sure any of them will/could be merged but I hope they will be at least reviewed.

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

Successfully merging a pull request may close this issue.

3 participants