LeonardoGentile / mobx-router5

Router5 integration with mobx
MIT License
22 stars 7 forks source link

fix: Added @action decorator to updateRoute and resetRoute methods of… #3

Closed aap82 closed 6 years ago

aap82 commented 6 years ago

… RouterStore as they directly m

coveralls commented 6 years ago

Coverage Status

Coverage remained the same at 74.419% when pulling 895e52c1c7770fdde3f33048e6fb988c9477a21a on aap82:aap82 into e44133c2719c27b05779b465365a0c89f8baa7ba on LeonardoGentile:master.

coveralls commented 6 years ago

Coverage Status

Coverage remained the same at 74.419% when pulling 895e52c1c7770fdde3f33048e6fb988c9477a21a on aap82:aap82 into e44133c2719c27b05779b465365a0c89f8baa7ba on LeonardoGentile:master.

LeonardoGentile commented 6 years ago

Thanks for this 👍 I will take a look and test it this weekend

LeonardoGentile commented 6 years ago

I would say this was not necessary because the two methods updateRoute and resetRoute are always called from other methods already decorated with @action, isn't it?

Do you have the need to call these two methods from outside the store instance?

aap82 commented 6 years ago

You're right that it's probably not necessary, but it just made sense to me to have it wrapped in the event that I or anyone else want to use it directly. But no, I do not have the need for it at the moment.

On a side note though, I think that these

  @observable route = null;
  @observable previousRoute = null;
  @observable transitionRoute = null;

should be actually be

  @observable.ref route = null;
  @observable.ref previousRoute = null;
  @observable.ref transitionRoute = null;

because the route properties themselves are never mutated, as they are replaced in whole. So, there isnt really a need to mark the properties observable, just the reference. Thoughts?

LeonardoGentile commented 6 years ago

I'm closing this as updateRoute and resetRoute are intended as helpers and always called from other @action decorated methods.

I will implement the @observable.ref soon