IjzerenHein / firestorter

Use Google Firestore in React with zero effort, using MobX 🤘
http://firestorter.com
MIT License
378 stars 50 forks source link

computed props being called even though no change #105

Closed RWOverdijk closed 4 years ago

RWOverdijk commented 4 years ago

For some reason my computed prop is being called when a different prop on the same document changes:

export class Venue extends Document {
  @computed
  get activeMenu () {
    console.log('Called: ' + this.data.activeMenu);

    return new Menu(`menu/${this.data.activeMenu}`, { mode: Mode.Auto })
  }
}

For context: "activeMenu" is the id of a different doc from a different collection.

There's a "name" on the same Document. When I change it, the activeMenu getter gets called.

  1. Is my approach to have a related document wrong? Or is this computed prop not that weird?
  2. Why would changing the name trigger activeMenu to be called again?

For more context, the hasData prop on the document is false for a short amount of time when I change the name, too.

I feel like I'm missing something really basic, or that I've misunderstood some core concept for a while now.

damonmaria commented 4 years ago

Guessing as can't see the full context of what you're doing but convert activeMenu into a field and calculate the path, like:

activeMenu = new Menu(() => `menu/${this.data.activeMenu}`);

The Document will handle the reactivity for you.

Try to avoid side-effects (like creating Documents) inside @computed, that generally leads to weirdness and pain.

RWOverdijk commented 4 years ago

But if I remove the computed prop, every change seems to return a new instance of Menu (Menu also extends Document).

What would be the best way to set something like this up then?

damonmaria commented 4 years ago

Just to make sure we're on the same page I was suggesting the following (my previous comment was short as was typed on phone):

export class Venue extends Document {
  activeMenu = new Menu(() => `menu/${this.data.activeMenu}`);
}

If that is still giving you issues then there might be a complication with having it inside another Document. But I'm totally guessing there.

As a general rule I'd be wary of having links like this. If you're trying to do a relational DB style join then that's not really what Firestore is designed for.

RWOverdijk commented 4 years ago

Ah! That's new 😄 I've never used this constructor method with documents before, sorry. How does it work/what does it do?

A link to the correct documentation page would also be much appreciated, I can't seem to find it 😅

damonmaria commented 4 years ago

https://ijzerenhein.github.io/firestorter/#/./guides/SourcesPathsAndReferences?id=reactive-path-functions

This applies to both Collections and Documents. Pretty cool eh?

RWOverdijk commented 4 years ago

Well, this works! 🤯

Wait, is this using computedFn by any chance?

damonmaria commented 4 years ago

Dunno what computedFn is :) If this answers your question then can you close the issue.

RWOverdijk commented 4 years ago

It's a function from mobx-utils https://github.com/mobxjs/mobx-utils

It's like computed, but for methods.

damonmaria commented 4 years ago

Thanks @RWOverdijk. Lots of new stuff in there since I last looked.