Track nested changes

[antitoxic]

As per #2 this enables tracking nested changes without explicitly wrapping them in remotedev().

A few notes:

1. I'm not sure why the loop in send() was removed. I brought it back. It's used. I need feedback on this.
2. I'm not sure what

if (!onlyActions[objName]) {
  schedule(objName, { ...action, type: `┏ ${action.type}` });
  send();
  schedule(objName, { ...action, type: `┗ ${action.type}` });

was doing. I removed it. I need feedback on this. However to me this doesn't make sense.

[zalmoxisus]

I'm not sure why the loop in send() was removed. I brought it back. It's used. I need feedback on this.

Originally, I thought that reports for actions doesn't have spyReportEnd, that's why I introduced the loop. Looking into mobx-devtools code, I found that actions reports are finished after intermediate updates, in order to create a group. So, I've implemented this here as well.

Before, we either show the action before the updates and display an inconsistent state, or show it after the updates (with the final state) and miss the point when an action is started. As everything except scheduled reactions has spyReportEnd, the loop doesn't make sense anymore.

Also, in that implementation neither toggling nor time travelling will not work correctly when having more than one intermediate update, as the action state is not final.

[zalmoxisus]

Tried with examples, and it's amazing!

Unfortunately, we'll not be able to dispatch actions remotely and autogenerate tests for nested observables. I would suggest to use this under a flag, let's say nested, similarly to new introduced onlyActions. So we can use the most convenient approach for different cases.

[antitoxic]

I was thinking about a flag too :) . @zalmoxisus do you use gitter? I wish for something more interactive because I think we need to clear a thing or two.

I'm not sure about the intermediate actions you're mentioning. The reaction() should be triggered once all changes are applied to an observable. And do you actually care about things like splice being in the time travel log? I think this is clutter and actually it's not an action, it's an operation which has little to do with the business logic.

Also what's the problem of dispatching actions remotely if they are just functions?

[zalmoxisus]

And do you actually care about things like splice being in the time travel log?

It can be disabled using onlyActions flag, but we show them by default as users may not use actions at all in their projects (for example, removing a todo in todomvc example has only splice). We don't want to use this lib for strict mode only.

I'm not sure about the intermediate actions you're mentioning.

I meant those things like splice you mentioned. Let's call them intermediate updates, and actions would be only functions with mobx.action. The confusion was because in terms of redux devtools, those updates are also actions.

Also what's the problem of dispatching actions remotely if they are just functions?

When I dispatch addTodo it is easy to call store.addTodo, but when I dispatch toggleStatus we still need those nested instances as before.

The reaction() should be triggered once all changes are applied to an observable.

In case of actions it's easy, but when they are not used, how will we know when there are no more changes to be applied?

@zalmoxisus do you use gitter? I wish for something more interactive because I think we need to clear a thing or two.

Yeah, feel free to ping me there, but still better to keep all the discussion here in case some one else has any suggestions.

[antitoxic]

It's a bit a more clear now. Thank you.

I think there are 2 distinct scenarios:

1. strict mode
2. and not strict mode.

In strict mode, nobody will want to see intermediate actions. I think we agree on this, right? The developer would expect timetravel to move through actions, not intermediate changes.

In contrast when not using strict mode you will always want to see intermediate actions.

Therefore the onlyActions flag seems obsolete. mobx-remotedev should detect whether mobx strict mode is enabled and switch behavior based on that.

About toggleStatus() - I think it's a confusing example. Having actions embedded in stores seems like antipattern. Actions should depend only on arguments, not inner state. So you can still execute toggleStatus() if it was accepting the todo as argument like so toggleStatus(todo).

In case of actions it's easy, but when they are not used, how will we know when there are no more changes to be applied?

What do you mean by that? The reaction will trigger once the observable changes and this is when the actions will be send to the extension. Is there a problem that I'm not seeing?

And the last thing you pointed out is generating tests. Can you give me a simple scenario where nested store wouldn't work? Even if that is the case, shouldn't we focus on the main goal of the extension - to help debug. Generating tests is a perk, not the main goal. Comparing nested feature VS tests I would go with nested to be default and if people want tests to enable non-nested behaviour.

Thoughts?

[antitoxic]

PS: __mobxGlobal.strictMode global variable denotes whether strict mode is used or not.

[zalmoxisus]

In strict mode, nobody will want to see intermediate actions. I think we agree on this, right?

There would be still useful to investigate changes for performance reason. There could be hundreds of changes in one action. Also, even in strict mode, the async changes can be outside the action (if runInAction is missed) and there will be no warning, and no info on the monitor. Another edge case is when setting strict mode to false for some part of code.

About toggleStatus() - I think it's a confusing example. Having actions embedded in stores seems like antipattern. Actions should depend only on arguments, not inner state. So you can still execute toggleStatus() if it was accepting the todo as argument like so toggleStatus(todo).

We better discuss it later, or you can try to implement it. Dispatching { type: 'addTodo', arguments:[...] } should work now.

What do you mean by that? The reaction will trigger once the observable changes and this is when the actions will be send to the extension. Is there a problem that I'm not seeing?

Then it should work great.

And the last thing you pointed out is generating tests. Can you give me a simple scenario where nested store wouldn't work? Even if that is the case, shouldn't we focus on the main goal of the extension - to help debug. Generating tests is a perk, not the main goal. Comparing nested feature VS tests I would go with nested to be default and if people want tests to enable non-nested behaviour.

Action { type: 'toggleStatus', arguments: [true] } will be turned into store.toggleStatus(true) for tests. Not sure we can identify that it should be for example store.todos[2].toggleStatus(true). The same problem I meant above for dispatching actions remotely. But, yes, we can just keep nested flag to false if there's need for tests.

[zalmoxisus]

PS: __mobxGlobal.strictMode global variable denotes whether strict mode is used or not.

Thanks for the info! We can then use onlyActions flag as true by default for strict mode, but still allowing to set it to false if necessary.

[antitoxic]

I'm testing things. Seems like even the current code (before my pull request) doesn't handle async actions.

if (change.type === 'action') {
  const action = createAction(change.name);
  if (change.arguments && change.arguments.length) action.arguments = change.arguments;
  if (!onlyActions[objName]) {
    schedule(objName, { ...action, type: `┏ ${action.type}` });
    send();
    schedule(objName, { ...action, type: `┗ ${action.type}` });
  } else {
    schedule(objName, action);
  }
} else if (change.type && mobx.isObservable(change.object)) {
  schedule(objName, !onlyActions[objName] && createAction(change.type, change));
}

If a new actions is run before the previous async competes you will get it in the middle of
┏ ${action.type} and ┗ ${action.type}

[zalmoxisus]

@antitoxic, don't see how this could happen, should be other problem there. Any steps to reproduce it? Modified this line with:

    this.props.store.increment();
    this.props.store.incrementAsync();
    this.props.store.increment();

and works well regardless if changes are wrapped in runInAction or not.

Is the same problem present in mobx-react-devtools logs?

[zalmoxisus]

@antitoxic, that issue should be fixed in 9aca4ab.

I've just published a new version of the extension, and we're ready for 0.1.0 release here. All the monitor features including remotely calling of the class methods are implemented. If you can give it a try, your feedback would be much appreciated.

Thanks to @mweststrate, now we can call useStrict() to see if it is strict mode, and set onlyActions to true by default. But we better not use it now, as calling this function will set strict mode to false for earlier versions of mobx. Maybe we could have a mobx.isStrict() method?

Regarding this PR:

1. We need removed part back.
2. Having nested parameter with the default value to false (if it proves to handle all cases, we'll switch to true by default).
3. It would be great to include the path for actions, instead of toggleStatus type to have todos[1]. toggleStatus. It would allow skipping actions and autogenerating tests. We can try to implement it after this PR, as I have no idea how we can get the path.

[zalmoxisus]

Added another implementation without using reactions in #5.

[antitoxic]

I'm getting back into this :)

Few things:

• acitons are not required to be part of stores. We cant expect actions to have a "path". Even more actions can be pure functions, totally generic like toggler(toggable) or addressPrefixer(address). If that is true, do you know what part of mobx-remotedev features become totally impossible? (skipping actions, test generation, etc?)
• capturing async sub-actions (update, splice) is still mystery for me. I can't make it work.

[zalmoxisus]

Yes, it makes impossible only skipping actions and test generation. Also it makes debugging useless in some cases. For example, you will not be able to figure out why your toggleStatus didn't work if you don't know how to call it (if it doesn't work only for certain todos).

[antitoxic]

For example, you will not be able to figure out why your toggleStatus didn't work if you don't know how to call it (if it doesn't work only for certain todos).

That's true. It's easier if it's part of a object because there is no parameters, but couldn't we just dump the state as JS object and get stacktrace? That should be enough to find out how to call toggleStatus. Am I missing something?

[zalmoxisus]

Yes, for mobx-react-devtools logs it would help. But here I don't think we want to store every stack trace. Note that the problem could be up in the history tree.

[antitoxic]

I'm trying to figure out the purpose of:

          schedule(objName, { ...action, type: `┏ ${action.type}` });
          send();
          schedule(objName, { ...action, type: `┗ ${action.type}` });

And when I run:

appState.incrementAsync()
appState.increment()

This is what I see:

So Timeout increment is not wrapped in incrementAsync. + having ┏┗ adds steps in the slider monitor which don't affect anything.

Is this the supposed visual output?

[antitoxic]

To clarify: I'm asking the above in relation to async actions. I understand the purpose for sync actions.

[zalmoxisus]

Is this the supposed visual output?

I think yes. Timeout increment could start in several hours, but we want to show other actions on the monitor. Normally, besides async actions you could also have some sync changes inside incrementAsync actions. Showing it as empty, signals that nothing was changed.

having ┏┗ adds steps in the slider monitor which don't affect anything.

Yes, we could skip them, modifying the slider monitor. Also we could modify the inspector monitor to make them collapsable if that makes sense.

However, the recommended way is to use strict mode, and then we'll see just actions names.

[antitoxic]

After some digging, I am pretty sure mobx makes it impossible to know all the stores that an action alters (cc: @mweststrate). By which I mean there is no way to figure out all affected parent observables even if we have reference to the currently affected observable. That's kind of sad.

Relying on reaction has proven to be uncapable of tracking async actions that are just that - async, no sync changes.

I'm closing this. Please reopen if you have other ideas. I will now open a new pull request - #6 . Very simple one - an option to set a "global default store" which will be used to capture and show all uknown changes in the remotedev extension.