Make params object observable and make installable from github with jspm

[salfield]

Hi Leonardo,

Thanks, for the module, I think there is quite a nice fit between mobx and router5 state driven routing - so started using this.

Not sure if this interesting to you, but I made the parameters into an observable map. I was trying to 'observe' params.x and expected that since params was an observable object that it would change on update. It doesn't because it was completely replaced. So I turned it into an observable map.

Let me know if you are interested in merging, or if I should carry on with my fork?

Many thanks,

Tom

[LeonardoGentile]

Hey @salfield thanks for that!
I'll review it tomorrow and also check why the build is failing 👍

[LeonardoGentile]

@salfield I've taken some time to review this.

Your updateRoute function seems to make things a bit unclear, and I'm not sure it sets the correct observables, it indeed breaks the tests and linting, have you tried to run npm run test and npm run lint?

Also as specified in the CONTRIBUTING.md this repo uses semantic release so the commit messages should respect that convention.

Ultimately, wouldn't a simple computed work or do you think a map is more interesting for a specific use case? Would you elaborate on this?

@computed params() {
    return this.route.params;
}

[LeonardoGentile]

@salfield I've written a test about the issue (are route.params observable?), could you please take a look at it here: https://github.com/LeonardoGentile/mobx-router5/blob/dev/test/main.js#L86

You can checkout the branch dev and run npm run test to check this.

It seems to me that the plugin works as expected and the route.params is an observable and so an autorun function will react to it on any param change.

Am I missing something?

[salfield]

@LeonardoGentile apologies for not following the contributor guidelines - it was just a very quick customisation for my purposes and given the size of the package I'm happy to maintain my fork.

route.params are certainly made into an observable object my mobx. However, they are replaced on each route (including params) change, so actually never change. This means that if in a react component I pass in route.params <Component params=route.params /> and in the component I access params.varName a change will not cause a re-render of the component. Further since the params object is an observable object, you wouldn't be able to add new params after constructing the route (except for the fact that you reconstruct them on each change). The advantage of an observableMap is that you can dynamically add observable keys.

I guess you would expect that the parent component would re-render, re-rendering the child. However it doesn't, as I'm doing a componentShouldUpdate check based on intersectionNode (similar to your own) in RouteNode. I pass route.params and route.name down from my RouteNode into the child component. Maybe I wouldn't have this problem if I used your react-mobx-router5 package, however I didn't understand the intended way that it should be used - nor the intention behind some parts of the implementation.

Your test dereferences the routerStore in the autorun with: let route = routerStore.route;. So, will re-run whenever route is replaced. The use case I am suggesting is where the params are dereferenced before being passed into a view, so the access params.varName would need to be re-run by an autorun.

Its true that if we put the suggested @compute params function on the routerStore, it would be possible to use the params like routerStore.params, and the would update when changing. However, how would you then differentiate the params on route v's transitionRoute, previousRoute etc.

[LeonardoGentile]

This means that if in a react component I pass in route.params and in the component I access params.varName a change will not cause a re-render of the component.

I see your point now, although there is a bit of confusion because this has nothing to do with the components of react-mobx-router5 which is a separate package.

I have tried to build on top of your solution but I'm not really convinced because all edge cases should be covered.

Rewriting a bit of your function updateRoute:

updateRoute(routeType, route) {
    const routeToUpdate = this[routeType];
    if (routeToUpdate === null ) {
      this[routeType] = route; // could be null or an object
      // init
      if (route) {
        this[routeType].params = observable.map(route.params);
      }
    }
    else {
      // you forgot yo copy over the meta and the path carried by the route
      Object.assign(routeToUpdate, {name: route.name, meta: route.meta, path: route.path});
      routeToUpdate.params.clear();
      for (let key of Object.keys(route.params)) {
         routeToUpdate.params.set(key, route.params[key]);
      }
    }
  }

Basically we say:

A) if this[routeType] was null then assign the route to it and if route is not null then rewrite the this[routeType].params in order to build a new observable map.

B) if this[routeType] was already set and was not null then update its observable map params.

Even tough this might seems logic, whenever the routeToUpdate was previously null we re-create the observable map, falling in the same pitfall you mentioned initially (re-creating a new observable route each time). One solution might be instead of initialising all routeTypes as null we could initialise them as empty objects having a params observable maps (empty)

{
  params: {// observableMap } 
}

but then it complicates things because instead of simply checking against null we would check against an empty object having an empty params...I wouldn't do that.

Do you agree?

---

About you problem, I was thinking: using router5 the only way to change params is to navigate to a new (could be the same) route and passing the required params (unless I'm mistaken). So if you think about it each param change is always triggered by a route change. So your initial concern about observing route instead of param shouldn't really matter. Just obser the route and the params should always be in sync.

So using my initial implementation why don't you just pass down the route observable and use the params inside the component?

// instead of this
<Component params=route.params />

// use this
<Component route=route />

@observer
class App extends React.Component {
  render() {
     const param1 = this.props.route.params.param1;
     return (
      <div>{param1}</div>
     )
  }
}

And if you want to inject extra params (why would you?) just do it in route.params

[LeonardoGentile]

Ultimately take a look at this possible implementation that would satisfy both of our needs, what do you think?

https://github.com/LeonardoGentile/mobx-router5/blob/observable-params/src/modules/RouterStore.js

[salfield]

@LeonardoGentile yes, I really like the use of emptyRoute to avoid the null problem you mentioned earlier. This looks much better to me. With regard to when you might want to add new params - I'm thinking for example when the params represent url query parameters which appear to be supported by router5.

http://router5.github.io/docs/path-syntax.html

It seems to be to be quite likely that when using query parameters that you might want to react to the adding and removal of parameters, whilst the "route.name" itself has not changed.

[LeonardoGentile]

@salfield I've completed a final implementation of this but I decided I won't support it for now, feel free to grab the code at: https://github.com/LeonardoGentile/mobx-router5/blob/observable-params/src/modules/RouterStore.js

I'm explaining my decision: any state change from router5 should be considered a new route. This should be considered immutable from the standpoint of mobx because only router5 should be allowed to modify and emit a new state. Furthermore all the internals of a state (path, params, meta) will never change unless router 5 emit a new state. So I strongly think we should observe the routerStore.route as a whole. In fact I will probably modify this to be @observable.ref https://mobx.js.org/refguide/modifiers.html because nothing apart router5 should modify this.