@observable should imply @action for the setter

[csells]

It seems counter-productive and unintuitive to have a runtime exception when calling a setter on an @observable and ask people to write custom setFoo @action methods when the setter on the @observable is already being generated and can produce the correct @action behavior.

[csells]

It does seem like ReactiveConfig is made to allow me to change this policy:

MyApp() { // allow @observable setters to run mainContext.config = ReactiveConfig.main.clone(enforceActions: EnforceActions.never); }

Unfortunately, there's a bug in the clone method where enforceActions is a bool instead of an instance of EnforceActions, so I can't actually run this code until that bug is fixed.

I do still wonder why the default for enforceActions is EnforceActions.observable, which causes exceptions to be thrown when the @observable setter is called. Is this a purity thing?

[csells]

Updating to the latest version solves this type problem but in the current version, while I am allowed to write the correct code to change the policy, it still fails at run-time:

MyApp() {
    // allow @observable setters to act (except it fails at run-time)
    mainContext.config = ReactiveConfig.main.clone(writePolicy: ReactiveWritePolicy.always);
}

A assertion inside the code makes it clear that such a change in policy can't be allowed in "strict-mode":

        case ReactiveWritePolicy.always:
          assert(_state.isWithinBatch,
              'Since strict-mode is enabled, changing observed observable values outside actions is not allowed. Please wrap the code in an "action" if this change is intended. Tried to modify ${atom.name}');

I don't see any mention of "strict-mode" anywhere else...

[pavanpodila]

The strict-mode message is a vestige from the MobX.js world. This should be corrected to the right jargon of MobX.dart. If you want to avoid this exception, you could set it to ReactiveWritePolicy.never, but I think based on our discussion, we should be generating the correct action-wrapper inside the setters.

[csells]

Ah. "strict-mode" must be the "don't ever allow observables to change", which is set with ReactiveWritePolicy.always. I read "always" as meaning "always allow" instead of "always throw" aka "strict-mode". After RTFM, I was able to get to the correct code on the latest version:

MyApp() {
    // allow @observable setters to act
    mainContext.config = ReactiveConfig.main.clone(writePolicy: ReactiveWritePolicy.never);
}

I am curious, however, if the generated setter for an @observable should write the change in the same kind of transaction that an action is wrapped it. What protections/semantics does it provide? Are they important for a setter? Reading the code, it doesn't seem so:

/// Internally MobX uses a 2-phase change propagation that ensures no unnecessary computations are performed.

That doesn't seem to apply in the setter case.

[pavanpodila]

The role of Actions becomes pivotal when you have observing reactions. If an action modifies a set of observables, it will only notify at the end of the action. This keeps the changes atomic and resulting in fewer notifications as well. It is also a performance optimization to avoid a noisy/chatty reactive system.

If a setter is called on its own, it can seem like an overhead to wrap in an action.

One possible solution is to take in an argument for the @observable annotation to make setters as actions.

We haven't done this for all setters, as it adds unnecessary action wrapper and also encourages stray mutation of observables. However, I do see your point of convenience and DX getting compromised with a manual action wrapper.

@rrousselGit and @csells does that sound like a possible option?

[csells]

I definitely see the need for a global policy. I can see some developers who would want "strict mode", but I think it should be opt-in instead of opt-out. Perhaps a different default for the global write policy and then an argument to @observable to override the policy on a case-by-case basis? In that case, the argument to @observerable would default to follow the global policy.

[pavanpodila]

If I understand correctly, we should default to an action-wrapper for all setters and have a per-observable setting to not have this wrapper?

The current default is set to ReactiveWritePolicy.observed, which means it will throw an exception if a mutation happens outside an action when that value is being observed by a reaction.

[pavanpodila]

In the latest update, I removed the "strict-mode" text in the exception 👍

[csells]

If I understand correctly, we should default to an action-wrapper for all setters and have a per-observable setting to not have this wrapper?

The current default is set to ReactiveWritePolicy.observed, which means it will throw an exception if a mutation happens outside an action when that value is being observed by a reaction.

I was thinking about the run-time policy, but you're really asking about the compile-time policy. At compile-time, I think we should generate the action-wrapper by default in the codegen and provide an argument to @observable to turn it off as an optimization if you really know what you're doing.

[pavanpodila]

At runtime (in release mode) this won't be checked as this policy check is purely for debug-mode. We can do the action-wrappers by default with a setting to override on a case-by-case basis. I am wondering if that complicates the behavior?

[csells]

The simplest is to do away with the write policy and always use the action-wrappers.

[pavanpodila]

@csells, I have added this in this commit and published as mobx 0.3.2 and mobx_codegen 0.3.2.

You can also see the simplification of the todo.dart file where a markDone() action is no longer needed.

Note that we are not doing away with the write-policy, instead we are conditionally wrapping the setter in an action. This is a good middle-ground without wrapping every setter in an action.

Let me know if this is better.

[csells]

This is better; I like the use of conditionally applying the write policy. Can you apply this same logic to the observable collections? The semantics should be the same and the update simplifies the creation of data models with collections in them as well.

As an example, in my own Todo sample update, I'm able to remove the addTodo and removeTodo methods from TodoList, using the ObservervableList directly from my AddTodo widget.

[pavanpodila]

I'll see how I can do this without sprinkling the conditional logic over all collection methods. Will think this over.

For collections, I would argue that they are higher level operations and would benefit having a semantic action calling out the operation. addTodo is a good example where having that operation called out gives it more meaning.