Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Track nested changes with full path #5

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

zalmoxisus
Copy link
Owner

@zalmoxisus zalmoxisus commented Aug 2, 2016

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?

@zalmoxisus zalmoxisus mentioned this pull request Aug 2, 2016
@antitoxic
Copy link
Collaborator

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
Copy link
Owner Author

@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
Copy link
Collaborator

Few ideas:

  • can we check the value of this in the action?
  • can we check parameter values?
  • check parent as it is in this pull request.

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

@zalmoxisus
Copy link
Owner Author

zalmoxisus commented Aug 23, 2016

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
Copy link
Collaborator

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
Copy link
Owner Author

zalmoxisus commented Aug 23, 2016

@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
Copy link
Collaborator

antitoxic commented Aug 23, 2016

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
Copy link
Owner Author

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
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants