mobxjs / mobx

Simple, scalable state management.
http://mobx.js.org
MIT License
27.37k stars 1.76k forks source link

Cascading changes `Cycle detected` #64

Closed kmalakoff closed 8 years ago

kmalakoff commented 8 years ago

I have a case where I need to manage the lifecycle of domain objects (by calling destroy on them) but they are synchronized with another observable so I'm trying to cascade changes, but keep running into the Cycle detected error.

An example flow is:

update a setting with the list of projects -> create or destroy project domain models -> provide an array of current domain models

I originally implemented it like:

class Store
  @observable projects = [];

  constructor() {
   autorun(() => {
      let projects = this.settings.projects.map(path => _.find(this.projects, project => project.path === path) || new Project(path));

      _.difference(this.projects, projects).forEach(project => {project.destroy(); this.projects.remove(project)}); // destroy and remove old -> could have side effects
        this.projects.replace(projects); // `Cycle detected`!
      });
    })); 
  }

// somewhere else
let store = new Store();

So I replaced it with:

   autorun(() => {
      let projects = this.settings.projects.map(path => _.find(this.projects, project => project.path === path) || new Project(path));

      untracked(() => {
        _.difference(this.projects, projects).forEach(project => {project.destroy(); this.projects.remove(project)}); // destroy and remove old -> could have side effects
          this.projects.replace(projects); // removed `Cycle detected`
      });
    })); 

I spoke with someone about this and he suggested one solution could be to use a computed version of the projects (although he suggested simplifying by making the store an observable instead of a class and didn't add the outside variable):

let projects = [];

let store = observable({
  projects: () => {
     projects = this.settings.projects.map(path => _.find(_projects, project => project.path === path) || new Project(path));

      _.difference(this.projects, projects).forEach(project => {project.destroy(); this.projects.remove(project)}); // destroy and remove old -> could have side effects
      return projects;
  }
});

He suggested that because destroy itself could have side effects, that this wasn't a good solution since cycles could be created elsewhere.

I've been running into this a lot where something is observing another property and then needing to cascading update some other observables but when it gets triggered, it is in the middle of an update cycle so I get Cycle detected.

One solution is to try to make all dependent values computed, but with potential side effects, it seems like untracked is the best hammer for all nails?

When using KnockoutJS, I use kb.ignore and peek from time to time to stop auto-subscribing, but don't remember getting cycle warnings since it didn't really track them.

Question: how should cascading changes with and without potential side effects be set up in mobservable?

mweststrate commented 8 years ago

Some thoughts on this:

first of all, I think indeed your second solution is nice, after all, the projects is derived that and thus it shouldn't be state.

For the cleanup part, I would put the clean up in an autorun, because it is an action that needs to happen as side effect of changes in derived data. So that would look like this:

class Store {
    @observable get projects() {
       return this.settings.map // etc
    }

    _previousProjects = []; // not obsrevable!
    constructor() {
        autorun(() => {
              const newProjects = this.projects.slice();
              // diff & destroy
              this._previousProjects  = newProjects;
        });
}

What happens when destroy has side effects is a bit tricky. The times that I ran into such cases I usually discovered I didn't apply reactivity radically enough. Personally (professionally) I didn't need untracked yet. Usually I discover that destroying should be reactive in itself as well. autorunUntil is usually very useful here: "wait until a certain condition is met, then destroy". In your case that could be something ilke:

class Project {
    constructor(store) {
        autorunUntil(
            // if this yields true
            () => store.projects.indexOf(this) === -1
            // do that (and then stop the autorun)
            () => this.destroy();
        )
    }
}

With this approach you probably don't need the autorun in the store. In my flux-challenge solution I applied this pattern several times (see for example: https://github.com/staltz/flux-challenge/pull/50/files#diff-d3014615aa7d81c40ec426225337ccc5R164)

mweststrate commented 8 years ago

Btw I'm making serious progress on the reactive graph transformation thing, which can also call a cleanup method when an object is no longer used in the transformation.

kmalakoff commented 8 years ago

These 'pro tips' are exactly what I was looking for...I thought there might be a reactive way to do this!

I'd be happy to beta-test the graph transformation thing whenever you are ready...