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.shallow decorator #2437

Closed
Amareis opened this issue Aug 26, 2020 · 12 comments
Closed

@computed.shallow decorator #2437

Amareis opened this issue Aug 26, 2020 · 12 comments

Comments

@Amareis
Copy link
Contributor

Amareis commented Aug 26, 2020

There is @computed.struct, but there is no shallow decorator. But if computed getter returns array it may be really good to shallow compare it!

import { observable, computed, autorun } from "mobx";

class Some {
  @observable items = [];

  @computed
  get actives() {
    return this.items.filter((i) => i.active);
  }
}

let s = new Some();
autorun(() => console.log(s.actives));
s.items.push({ active: false }); //will recompute actives!

Yep, computed.struct will prevent recomputing, but for big arrays of classes it will be unefficient. As I see in https://github.com/mobxjs/mobx/blob/master/src/v5/api/computed.ts it'll be three strings change.

@Amareis Amareis changed the title @comuted.shallow decorator @computed.shallow decorator Aug 26, 2020
@urugator
Copy link
Collaborator

urugator commented Aug 26, 2020

computed({ equals: comparer.shallow })
#2247
#1076 (comment)

@Amareis
Copy link
Contributor Author

Amareis commented Aug 26, 2020

But it still not decorator?..

@mweststrate
Copy link
Member

mweststrate commented Aug 26, 2020

@Amareis see https://deploy-preview-2327--mobx-docs.netlify.app/refguide/computed.html, the section about computed.struct for some background, in general we don't recommend doing shallow or deep value comparisons at all, because typically they will always have a cache miss (see the explanation in that section).

In your case for example, by introducing a shallow comparison, you would make adding or removing items to the collection that don't match the filter cheaper (since then an updated collection would still result in the same output), but all other cases (like toggling active state) would be more expensive because you'd introduce a O(n) comparison which is never going to hit the cache. And even in the first case, where the shallow compare correctly would avoid an update, it depends on what happens downstream from the computed if doing the comparison was actually cheaper or more expensive than what happens downstream.

So it is fine to use those mechanisms, but beware that they can make things actually significantly slower instead of faster, so they shouldn't be applied without though, and the current api reflects that. (I think computed.struct maybe shouldn't be there either, but well, once something is out it is pretty hard to get rid of it ;-)).

@Amareis
Copy link
Contributor Author

Amareis commented Aug 27, 2020

For arrays shallow comparing is actually good enough, since length checks avoid O(n) comparison if array is really changed. And even with O(n) if array still the same it's better to prevent updating if where is some chained computeds (it's my case) or, even worse, some React components, depends on this array (my case too).

I understand your point, but, still, currently to avoid unessesary recomputes (for decorators) there is only @computed.struct, which is much worse than shallow.

@jezzgoodwin
Copy link

@Amareis are you aware you can already specify a shallow comparer with the decorator syntax?

@computed({ equals: comparer.shallow })

I thought I'd add my two cents on to this one as I frequently use shallow comparison. Just to be clear, I am completely happy using the @computed({ equals: comparer.shallow }) syntax, but from that amount I use it, I'd prefer to have computed.shallow available.

I often define objects which contain multiple properties ready for use in a UI component. For example:

interface IBook {
    title: string;
    isRead: boolean;
}

The values come from different observable data collections and these data collections store data for many books. I make a shallow computed function which maps the data to an IBook object:

@computed({ equals: comparer.shallow })
get book(): IBook {
    return {
        title: this.bookData.get(this.selectedId).title,
        isRead: this.readStauses.get(this.selectedId)
    };
}

If I used a regular computed function it would mean that any change to an underlying data collection would trigger a view update. Whereas using shallow comparison only triggers a view update if the particular book I'm interested in has changed.

@urugator
Copy link
Collaborator

urugator commented Sep 29, 2020

@jezzgoodwin
This seem a bit counterproductive ... when you merge these properties into a (new) single object, then your UI can no longer react to the changes individually. Whenever one of the properties changes it will invalidate everything that depends on this object, no matter whether it needs just title or whatever.

constructor() {
  this.selectedBook = observable({    
    get title() {
      this.bookData.get(this.selectedId).title;
    },
    get isRead() {
      this.readStauses.get(this.selectedId)
    },
  });
}
get book(): IBook {
  return this.selectedBook;
}

@jezzgoodwin
Copy link

@urugator your code is a better approach for the basic example I gave. I will keep this idea in mind.

Generally when I construct an object with multiple properties it's because I need to do some conditional logic and I don't want to have to repeat it. Eg, I wouldn't want to repeat my conditional logic within both the title and isRead getters.

Also when I make these types of objects, they are usually being used by a single view component. The component would need to rerender if either the author or isRead has changed. I don't usually need the fine grained granularity.

@Amareis
Copy link
Contributor Author

Amareis commented Sep 29, 2020

@Amareis are you aware you can already specify a shallow comparer with the decorator syntax?

Oh, really? Don't know it, thanks!

And i still think that this syntax deserves shortcut :D

@jezzgoodwin
Copy link

@Amareis presumably this issue could be closed? It doesn't seem like there is a strong backing for adding a specific decorator. Especially as the long form version is available.

@Amareis
Copy link
Contributor Author

Amareis commented Sep 29, 2020

Yep, thinks so.

@Amareis Amareis closed this as completed Sep 29, 2020
@mweststrate
Copy link
Member

For future readers, this section now explains why it is very rarely needed to do comparisons on the output of a computed; typically the output changed anyway otherwise the computed wouldn't have run in the first place: https://mobx.js.org/computeds.html#computed-struct

@Amareis
Copy link
Contributor Author

Amareis commented Oct 1, 2020

But computeds with array filter/sort is still good case for shallow comparing :D

Venryx added a commit to Venryx/mobx-graphlink that referenced this issue Sep 24, 2023
…underlying "computed" call. This enables caching for some situations that weren't easily possible before; for more info, see: mobxjs/mobx#2437 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants