Mobx: The controversial behavior of reactions when they are created within another reaction

[mass3ff3ct]

Hello. Sorry for my English :)

Example:

class TreeStore {
     ....
     constructor(settingsFactory: () => StructureType) {
           reaction(
               () => settingsFactory(),
               settings => {
                      this.setSettings(settings)
                      this.buildReqursiveList()
               },
               {
                      fireImmediately: true
               }
           )
      }

      @action
      private  buildReqursiveList() {
            // There is a flat hash map that contains all instances of the created lists
            // Create and add an item to a hash map:
           this.listHashMap.set(listHash, new ListStore(settingsFactoryForList)) // Nested reactions are created with fireImmediately = true
      }
}

The problem is that nested reactions are not immediately launched. Their startup is blocked by the handler runReactions.
This leads to unobvious consequences. I'm expecting a synchronous code (fireImmediately = true), but the result is not that

Is this normal behavior and should I refactor my code? Or is it a mobx error?

[urugator]

Here is the deal:
The sideEffect function needs data (settings in your case) provided from data function. So the data function have to run prior to sideEffect function.
The problem is that the data function can't run during the ongoing transaction (action). Why? Because the data function would read from potentially inconsistent state - it would run before the current transation (action) finishes.
In other words you can't mutate the state and read from the state at the same time.
So MobX waits until the transaction finishes and then runs the subscribers (data functions) in the order they were created.

Also note the following:

// Consider you're instantiating the TreeStore inside some action (transaction)
@action createTree() {
  const tree = new TreeStore();
  // at this point, the reaction have NOT run yet, the TreeStore is NOT yet initialized
  // the reaction will run after this methods ends 
  if (tree.listHashMap.has("something")) { // NOPE!
    // ...  
  }
}

EDIT: Another way to describe fireImmediately: true:
Don't wait for data modification, run the effects immediately after the data are available (all actions are finished)

[mweststrate]

this.setSettings(settings)
this.buildReqursiveList()

In general, producing values in reaction to some other value is an anti pattern in mobx. Try using computed values instead

[Strate]

It is unobvious with fireImmediatelly: true. I expect synchronous first execution of effect function with this flag.

[mweststrate]

It is synchronous, but still at the end of the current action. Reactions
are for side effects and never processed until the actions are completed

Op di 23 mei 2017 17:46 schreef Artur Eshenbrener <[email protected]

:

It is unobvious with fireImmediatelly: true. I expect synchronous first
execution of effect function with this flag.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/mobxjs/mobx/issues/1006#issuecomment-303441132, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABvGhMVqKoZpG1gbXftA_BH7QuvchoWWks5r8v9PgaJpZM4NhqLE
.

[urugator]

It is synchronous

I don't agree. Reactions should be always considered asynchronous. As a consequence of TX nesting, they work actually the same as asychronicity in JS.
Reactions are pushed to a queue the same way as callbacks are and proccessed after the current call stack (action stack in MobX) is finished.
So they're (a)synchronous the same way as JS is (not).

[mweststrate]

Interesting, I always call that synchronous as it originates from the same stack and there is no interleaving with other async JS calls. Like trampolining. But one could argue that that is async indeed. Probably I'll should check some official definition :-P.

Anyway, the important question: @mass3ff3ct did the above comments answer the question?

[Strate]

@mweststrate I can answer for @mass3ff3ct, because he is my teammate :) Yes, your answers helps, but it was not so clear when we ran into this issue. I think we could improve mobx in this way by:

1. Improve documentation of reaction and runImmediatelly, or
2. Invoke effect function when reaction created with runImmediatelly directly with reaction creation, or
3. Something else ;-)

[rchanou]

Is it possible for a warning, or even error, to be added for changing observables in a reaction, instead of silently allowing it without firing other reactions as one might expect? Seems like a huge gotcha.

[mweststrate]

No, this is as it should work, side effects are not supposed to be used in
such a way you might observe it in the first way in code. If you are
depending on the output of a side effect, it should not be modeled as
reaction but as computed value, which have the correct behavior in these
kind of situations. Side effects/ reactions are for printing, network,
drawing stuff etc

Op wo 24 mei 2017 22:11 schreef Ron ChanOu [e-mail]:

Is it possible for a warning, or even error, to be added for changing
observables in a reaction, instead of silently allowing it without firing
other reactions as one might expect? Seems like a huge gotcha.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/mobxjs/mobx/issues/1006#issuecomment-303837857, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABvGhLhDqxXYtILvQgHIYfUo_jpfHipRks5r9I8JgaJpZM4NhqLE
.

[urugator]

should not be modeled as reaction but as computed value
Side effects/ reactions are for printing, network, drawing stuff etc

Let me offer another guidlines for reaction:

The reaction is used to connect MobX based system with another system. It's completely fine if that another system is also a MobX based system, but in any case the following conditions must be met:

The systems must be:
DISCRETE

• they must not share any state

The communication must be:
ASYNCHRONOUS

• it doesn't matter if both systems are synchronous

UNIDIRECTIONAL

• the reads and writes are strictly separated and must not occur at the same time
• use the first data function to performs reads from the first system, writes are forbidden, accessing the second system is forbidden, use return value to provide data for writes
• use the second effect function to perform writes on the second system, reads are frobidden, accessing the first system is forbidden, use data argument to obtain the data from reads

Putting it to the code:

// SYSTEMS MUST NOT SHARE ANY STATE
const system1 = System1();
const system2 = System2();

const disconnectSystems = reaction(readsFromSystem1, writesToSystem2);

function readsFromSystem1() {
  /* 
  // WRITES ARE FORBIDDEN
  system1.action();
  // ACCESSING SYSTEM 2 IS FORBIDDEN
  system2.action();      
  if (system2.value) {...} 
  */  
  return system1.value
} 

function writesToSystem2(data) {
  /*
  // READS ARE FORBIDDEN  
  if (system2.value) {...}
  // ACCESSING SYSTEM 1 IS FORBIDDEN
  system1.action()
  if (system1.value) {...}
  */  
  system2.action(data);     
}

EDIT: Let me know if someone want to explain why these restrictions are neccessary and why they should guarantee that you're covered when using reactions.

[rchanou]

Sorry, I probably wasn't clear; I think I'm in agreement, I was just wondering if the anti-pattern could be detected and throw a warning, with a tip to use computed values instead.

[mass3ff3ct]

Thank you all for the prompt explanation of the problem.
I think that the issue can be closed:)

[HamsterHuey]

@mweststrate @urugator - I have a related question and figured I'd post here rather than open a new issue regarding this. From this issue and a couple others I've read where both of you have participated, it appears that mutating store state via a side-effect in a reaction is considered an anti-pattern / bad practice.

However, the MobX Quickstart Guide Book - co-authored by @mweststrate himself, seems to have several examples where the reaction mutates state in the store. For example: https://github.com/PacktPublishing/MobX-Quick-Start-Guide/blob/cbe2fe7b740ae3886a0078072a5265d58b8d5718/src/Chapter06/form-validation/store.js#L51

Here, the side-effect validateFields() is an async (flow) method that updates a few fields in the store. Similarly, the page routing example here - https://github.com/PacktPublishing/MobX-Quick-Start-Guide/blob/cbe2fe7b740ae3886a0078072a5265d58b8d5718/src/Chapter06/page-routing/CheckoutWorkflow.js#L107
has the autorun side-effect function mutating state.

So now I'm thoroughly confused about what actual best practices are. It seems a bit unavoidable to have some state mutation when using reactions for things like form validation. I believe I understand why this is recommended against since they can result in a cascade of nested state updates + reactions that can make reasoning about the code much harder, but I'm having a hard time understanding why the examples in the Quickstart Guide book are then considered okay? Perhaps there is some middle-ground where mutations/actions in side-effects are still okay, but I don't have a clear idea of how I can go about determining whether something is or isn't okay. I'd appreciate any clarity on this. The docs unfortunately don't seem to make any mention on this matter at all. Thanks!

[spion]

Personally, I prefer using fromPromise with computed, despite the flaws with tracking what happens after the first async request and despite the lack of cancellation strategies to go with it:

@computed get fieldValidation() {
   const fields = this.getFields();
   return fromPromise(Validate.async(fields, rules))
}

However in the example you posted, the validation happens as part of an action when the user clicks the "enroll" button. Unlike validation, enrolling is a side-effect. Therefore we will use flow. But we need the result of the field validation! No problem - we can yield the fieldValidation computed, which also satisfies the promise (thenable) interface. As a bonus we're not re-validating needlessly if the values haven't changed:

enroll = flow(function*() {
  yield this.fieldValidation;
  yield this.enrolling = fromPromise(enrollUser(this.getFields()))
}

Now you can render the enrollment result with a simple case on the promise based observable:

render() {
  this.fieldValidation.case({
    pending: () => render form disabled, with spinner,
    fulfilled: () => render form normally with fields in green
    rejected: () => render validation errors
  })
}

You can also render things on the page based on the state of the enrollment status:

render() {
  if (this.enrolling) {
    if (this.enrolling.state === 'pending') disable clicking enroll button
    else if (this.enrolling.state === 'rejected') render that enrollment failed
    else { dont render form, render a success callout instead }
  } else {
    enroll() flow not called yet, just render the form
  }
}

Note that you can use case on a computed fromPromise as many times as needed when rendering certain parts of the page.

You could go further and model flows as computed values too, its just a little awkward: https://github.com/mobxjs/mobx-utils/issues/90

IMO most uses of reaction() are a smell of some kind and can likely be removed, vastly simplifying code in the process.

[HamsterHuey]

Thanks @spion , that example makes sense and helps a lot. I did look into considering async computed values using fromPromise or the computed-async-mobx library. Your example makes it clearer as to how one might tackle this. The issue I currently have is that I can end up in situations where the async computed method could end up being called in quick succession and I'm worried about the older/stale Promise resolving later and overwriting the "latest" value. Does the above approach avoid that?

At the moment, I have something like the following in my store:

this.dataFetchPromise = null;

this.disposeAsyncFetchReaction = reaction(
  () => { data: this.someData },
  async (data) => {
    if (this.dataFetchPromise) {
      await this.dataFetchPromise.cancel();
    }
    this.dataFetchPromise = this.dataFetchFlowMethod(data);
    this.dataFetchPromise.catch((err) => { // Need this since cancelled promise is rejected with FLOW_CANCELLED error
      if (err.message === 'FLOW_CANCELLED') {
        console.log('Successfully cancelled stale dataFetchPromise');
      } else {
        console.error('Error in dataFetchPromise', err);
      }
    });
  }
)

dataFetchFlowMethod = flow(function* (data) {
  // Async fetch method that mutates some observables in the store
});

This way, I can make use of the cancel() method that is provided by MobX on promises returned from a flow() method. This works pretty well, but it does result in the reaction firing off an action (the flow method here) and mutating state. I'm kind of okay with it here since the observables being modified by the flow method aren't really touched by anyone else in the store (and only observed by the React component that displays the obtained data from the async fetch).

The async @computed approach is cleaner, but I'm not entirely sure how to handle concerns about race conditions.

[urugator]

Following is just my opinion or something to discuss/consider, not the best practice.

TLTR: Don't use reactions at all.

I will start with a code. Notice how react handles synchronization between components:

class Parent extends React.Component {
  constructor(props) {
    super(props);
    this.state = {
      foo: 0,
      bar: 0,
    }
  }
  setFoo(foo) {
    this.setState({
      foo: foo,
      bar: foo + 1,
    })
  }  
  render() {
    return React.createElement(Child, {
      ...this.state
      setFoo: foo => this.setFoo(foo),    
    })
  }
}

class Child extends React.Component {
  onClick() {
    this.props.setFoo(5);
    // Following "waits" for parent to update
    this.setState((state, props) => {
      console.log(props.foo, props.bar) // 5 6   
      return {
        sum: props.foo + props.bar; // 11
      }                   
    })
  }
  render() {
    return React.createElement('button', {
      onClick: event => this.onClick(event),
    })
  }  
}

Lets create similar parent-child relationship with mobx:

class Parent {
  @observable foo = 0;
  @observable bar = 0;

  constructor() {
    this.child = new Child(this);
  }

  @action
  setFoo(foo) {
    this.foo = foo;
    this.bar = foo + 1;
  }  
}

class Child {
  @observabl sum;

  constructor(parent) {
    this.parent = parent;
  }

  @action
  onClick() {
    this.parent.setFoo(5);
    console.log(this.parent.foo, this.parent.bar) // 5 6  
    this.sum = this.parent.foo + this.parent.bar; // 11
  }    
}

So far so good, what if we use reaction to keep the bar synchronized with foo

class Parent {
  @observable foo = 0;
  @observable bar = 0;  

  constructor() {
    this.child = new Child(this);
    reaction(
      () => this.foo,
      foo => this.bar = foo + 1,
    )
  }

  @action
  setFoo(foo) {
    this.foo = foo;    
  }  
}

class Child {
  @observabl sum;

  constructor(parent) {
    this.parent = parent;
  }

  @action
  onClick() {
    this.parent.setFoo(5);
    console.log(this.parent.foo, this.parent.bar) // 5 0 (the reaction haven't run yet)
    this.sum = this.parent.foo + this.parent.bar; // 5
  }    
}

The problem could be easily solved with computed, however I wanted to demonstrate that in general it's not possible to safely transfer one state to another via reaction and ensure consitency at the same time.
I think that people tend to keep their models/stores quite simple and flat and reactions relatively isolated, so the problem actually rarely occurs.
However I think it's good to know about this, because then you may also realize why setState in react is "async" by design - the child component must "wait" for the parent to update to keep the state consitent. While in mobx state modifications don't wait for sheduled reactions as that would kill the point of transactions. ("async" is intentionally in quotes, because everything runs sync, things are just not executed immediately at callsite - dunno how to put it better)
It's also interesting to think about what happens when updating parent affects the existence of child. It may give you a hint why it can be useful to normalize the state.

There are two (generalizable) situations where the above problem will never appear:
1) Using reactions exclusively to communicate - this has the same effect as not using transactions. Probably not very useful, but perhaps it could be used to communicate between 2 mobx based systems each with own transaction boundaries.
2) When reaction's effect is an async operation - in that case the immediate consistency is impossible by the nature of the operation. The consistency must be ensured manually later (when operation resolves - eg by checking timestamps on response).
Such operations can't be batched (wrapped in single transaction) therefore the probem can't occur.

So let's say you would use reactions exclusively for async operations. Then the question basically boils down to whether you want the effect (async operation) to be coupled with state or with the cause (action/event/timeout):

// Effect coupled with state (this.value)
reaction(
  () => this.value;  
  value => validate(value); 
)
// VS
// Effect coupled with cause (setValue)
@action setValue(value) {
   if (this.value === value) return;
  this.value = value;  
  this.validate(value);      
}

I always prefer the second approach because:
It doesn't introduce anything new.
It's immediately visible which action leads to which effects and how the data flows.
Therefore it's easier to follow, debug, understand and harder to introduce an error.
It doesn't require disposal management.
It's easier to compose - it's just a function after all - you can reuse it, intercept it, chain it, consume returned value, whatever...
It doesn't overload the meaning of assigment this.value = value. Notice that with reaction you can't introduce an action which could change the value without causing validation (you would have to introduce another state used as condition, making it even more complicated...).
Reaction doesn't have an access to invocation context, everything it accesses must be part of the state (how do you pass parameter to reaction)?
Error handling is easier (reactions have shared centralized error listener).

There is also some similarity between componentDidUpdate and reaction as both are used to trigger side effects in the result of state mutation. So maybe that fact can also be used when deciding whether the use of reaction is appropriate (effect must be still async though!).

[spion]

Does the above approach avoid that?

Yeah - when the computed updates the old promise (and its extension fromPromise) reference gets discarded and replaced with the new request.

You can check if there is a pending request and disallow another enrollment during that time:

enroll = flow(function*() {
  yield this.fieldValidation;
  if (this.enrolling && this.enrolling.state === 'pending') return;
  yield this.enrolling = fromPromise(enrollUser(this.getFields()));
}

In the render function you can also disable the button in the UI based on this.enrolling.state.

The only thing that doesn't quite work is that for the validation, the previous computed result doesn't get cancelled, so all its requests will continue running (even though they're not used anywhere)

[HamsterHuey]

Wow! Thanks so much for the awesome replies @spion and @urugator . I am really grateful to both of you for taking the time to provide such detailed replies with very clear examples. I learned a lot from them! 👍

@spion - Your reply makes sense. I'm definitely going to try and use this approach to minimize the amount of mutable state and avoid having to rely on reactions.

@urugator - Your comments and example really helped highlight the drawbacks of trying to sync state via reactions and I believe I have a better understanding for why it would be better to avoid this.

If I could ask a practical piece of advice that might perhaps help me better understand how to consider re-architecting things with this way of thinking:

The application I am currently working on enables a user to interactively create, and edit a graph data structure. So they can specify nodes (with associated node metadata), edges, and connect/delete nodes/edges. I have a main store that handles the graph datastructure along with a class/store for Nodes and Edges. When the user deletes a node, I still want to keep it in the datastructure with a 'deleted' flag and will only delete/garbage collect it from the datastructure when there are no more edges that point to the 'deleted' node.

The easy way I took, was to have a reaction that essentially monitors for any mutations in the graph datastructure representation and fires an action deleteUnlinkedNodes() that checks all the nodes to determine if they can truly be deleted, and mutate the graph accordingly if necessary. This seems easy to reason about too, but, as you pointed out, this does not guarantee consistency. I am not guaranteed that my graph datastructure will never have a node flagged as 'deleted' that has no edges pointed to it. This could happen for fleeting moments (or not so fleeting moments) when the reaction has not yet fired.

From your comments above, the way I see to avoid this, is to ensure that I call my deleteUnlinkedNodes() action in tandem with any action that mutates the graph state... for example in the deleteNode, deleteEdge, saveActiveNode and saveActiveEdge methods. The former approach seemed "cleaner" in some sense because I don't have to keep mental tabs of every place in my codebase where I mutate my graph datastructure via an action and don't end up with errors where I forget to include a call to deleteUnlinkedNodes() in some action that mutates the graph state. Is there a more elegant way of being able to ensure that my deleteUnlinkedNodes() method can be called when I mutate my graph datastructure? Each approach seems to have tradeoffs, and I can certainly see why anything that allows your store to have (however fleeting) inconsistent state should probably be steered clear of, but in my situation, I'm not sure if that is completely warranted?

If it helps, I will probably be pushing this code to a public repo in the next couple of days.

[urugator]

@HamsterHuey I think it's always better solved with correct abstraction and composition. In your case you just need to intercept certain calls (graph mutators). Eg:

mutateGraph(mutator) {
  mutator(this.graph);
  this.normalizeGraph(); // deleteUnlinkedNodes()
}

deleteNode() {
  const mutator = () => {}
  this.mutateGraph(mutator);
}

// or statically
constructor() {
  this.deleteNode = this.createGraphMutator(this._deleteNode)  
} 

[HamsterHuey]

Great point @urugator ! That certainly makes sense. Thanks again for engaging in this great discussion. I really appreciate it. Cheers!

[lock[bot]]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs or questions.