zalmoxisus / mobx-remotedev

MobX DevTools extension
MIT License
327 stars 40 forks source link

[RFC] Track nested changes with full path #5

Open zalmoxisus opened 8 years ago

zalmoxisus commented 8 years ago

As discussed in #2 and #3, we want to track nested changes without explicitly wrapping them in remotedev(). In order to help debugging, to cancel (skip) actions and to autogenerates tests, we also want to add the path to the action name like following:

screen shot 2016-08-02 at 9 54 33 pm

This PR is just a proof of concept, it works only for arrays added with splice. We need to handle other observable types and update types. There's a lot of work, so feedback is welcome. Maybe we could get some information directly from MobX?

antitoxic commented 8 years ago

Just checkedout master and merged with nested.

Works with the "simple todo" example but only if the action is part of the Todo store.

If it's extracted like so:

var toggleStatus =  mobx.action(function toggleStatus(todo) {
  todo.finished = !todo.finished;
});

it doesn't work.

zalmoxisus commented 8 years ago

@antitoxic, thanks for checking this out.

That's because change.target is undefined in this case. The same problem is for mobx-react-devtools, it doesn't show for which observable is this action.

For example, runInAction has the third argument scope, where you can specify the class, but for action I don't see how this can be solved.

Maybe @mweststrate has any clue?

antitoxic commented 8 years ago

Few ideas:

If any of those are observables use them as parent path.

zalmoxisus commented 8 years ago

I don't see how we can solve this from our part. There should be found a way on MobX part to pass (or better to detect) the target. Especially because it also makes mobx-react-devtools logs useless for such cases.

antitoxic commented 8 years ago

I don't see how we can solve this from our part

Well if we can't find a target and one of the parameters passed is an observable, we can make a pretty good guess that the observable parameter is the target.

Same with this.

zalmoxisus commented 8 years ago

@antitoxic, I don't get it. We have only this information:

{type: "action", name: "toggleStatus", target: undefined, arguments: Array[1], spyReportStart: true}

I added you as a collaborator, you can make changes to this branch, so we can test it together.

Honestly, as discussed with @mweststrate, I don't see any advantages of nested observables / classes, and don't use them. However, it would be great to get it work, so we can prove we can debug everything in MobX as in Redux.

antitoxic commented 8 years ago

Honestly, as discussed with @mweststrate, I don't see any advantages of nested observables / classes, and don't use them.

Ok. Consider this as just some brain teaser. I don't want to bother you with something you are not keen on. It's your project so it should serve you in the first place :)

I added you as a collaborator

Thank you. Could be useful.

{type: "action", name: "toggleStatus", target: undefined, arguments: Array[1], spyReportStart: true}

Finding target from arguments if it's missing:

function getTarget(change) {
  var target = null;
  if (change.target) {
    return change.target;
  }
  for (let i = 0; i < change.arguments.length; i++) {
    if (mobx.isObservable(change.arguments[i])) {
      return change.arguments[i];
    }
  }
}
zalmoxisus commented 8 years ago

I don't want to bother you with something you are not keen on. It's your project so it should serve you in the first place :)

If it was supposed to be only for my needs, I wouldn't opensourced it :) Your suggestions are welcome!

Finding target from arguments if it's missing.

That would do the trick indeed. I missed that we pass the observable as an argument.

antitoxic commented 8 years ago

In relation to #6 and because you (@zalmoxisus) were asking, I think the work needed to finish all scenarios for this pull request is not worth the effort right now and even more I don't think it's possible unless @mweststrate provides solution to walk up observable hierarchy.

The string splitting of debug name seems unreliable.