Separate route matching from rendering #613

marcbachmann · 2017-12-27T00:25:43Z

This PR

Fixes the routing to modify the state before the navigate event happens. This means you can access the updated state within the navigate event. Modifies the state before the render event is triggered. Therefore this fixes State out of date when switching routes #530, Fire route event after render if the url has changed #553, state.query not set until navigate #549, Navigate event should enable action on "page entry" #610
Exposes .emit as an alias to .emitter.emit
Theoretically supports nested routes 😁 . But because we didn't document nor test the router in this repo, I'd handle that as private api for now.

Additionally to the changes I did in here I think we should

Define what's the responsiblity of nanorouter & wayfarer and how we could modularize it better
(I guess this is done by now as nanorouter contains some electron-, and browser specific logic)
Deprecate curry support in nanorouter. People could just wrap their method by themselves

Please let me know what you think

Before merging this PR, we have to cleanup the nanorouter dependency.
We'll also need to rename some methods and expose the _handler differently on the state if we decide to go down this path. In case we want to be able to cancel a navigate event in the future, we could also make it more stateless and pass the new route as object to the navigate events instead of modifying the state.

marcbachmann · 2017-12-27T00:41:44Z

index.js

-  this.state.query = nanoquery(location)
-  var html = this.router(location)
+  var href = location.replace(/\?.+$/, '')
+  this._matchRoute(href, location)


We could move this into the pushState/popState/replaceState methods.
But there might be other side effects if we do that.

e.g.

var route = this._matchRoute(href) self.emitter.emit(self._events.NAVIGATE, route)

or this (if we keep the mutation of state)

this._matchRoute(href) self.emitter.emit(self._events.NAVIGATE)

Perhaps we should leave this the way it is for this PR, and consider the changes in a seperate PR? That might keep the changes nicely contained, and easier to follow along.

Yes, I completely agree here. This definitely needs more elaboration.

The question here was whether we should prevent _matchRoute from mutating the state. It should only return the new params and mutate the state after the NAVIGATE event got triggered.
This would allow us to keep the current state but already pass route params to some function to preload something.

This could end up in something similar to the angular resolve object where it's possible to preload some resources based on route params. In there we could also cancel the route change if a resolve gets rejected.
Maybe this would make everything more complicated. I'll think about it and will create a new issue if I think it's a good idea in the longer term. Here's a link to the resolve reference documentation in case you don't know it:
https://angular.io/api/router/Resolve

And a few more readable examples:
https://www.codementor.io/max_diachenko/angular-2-route-resolves--typescript-du1089pdm
https://medium.com/@dazcyril/resolving-data-in-angular-2-4-and-5-refactoring-components-and-moving-to-ngrx-store-15e15d8be3dd#9968

yoshuawuyts

nice! Yeah, also all for deprecating the curry statement in nanorouter.

marcbachmann · 2017-12-27T14:48:42Z

btw. Don't merge just yet 😉

marcbachmann · 2017-12-30T17:08:37Z

index.js

+
+Choo.prototype._prerender = function (state) {
+  var routeTiming = nanotiming("choo.prerender('" + state.route + "')")
+  var res = state._handler(state, this.emitter.emit.bind(this.emitter))


Should we cache that this.emitter.emit.bind(this.emitter) variable somewhere? e.g. in the Choo constructor?

Yeah, that'd be great!

Directly as choo.emit? Then we would have choo.emitter and choo.emit.

marcbachmann · 2018-01-02T16:39:19Z

This is ready now for another review. We could wait for some hours until choojs/nanorouter#13 is merged. With that we would have a simplified nanorouter module by removing the curry option.

I'd also consider renaming the new methods to make the code more readable. Better names for the prerender method might be generateDomTree or toDomTree but that might be confusing if the server side function calls that and returns a string instead. So maybe generateView suits our use-case better.

Let me also know if you want to make the _prerender and _matchroute methods private.
Other modules like nanolocation are also exposed but I don't know for what reason ... maybe it's just a preference to keep the code simple. 🤷‍♂️

For the _prerender case we could use something like that:

@@ -189,7 +189,7 @@ Choo.prototype.toString = function (location, state) {

   var href = location.replace(/\?.+$/, '')
   this._matchRoute(href, location)
-  var html = this._prerender(this.state)
+  var html = prerender(this.state, this.emit)
   assert.ok(html, 'choo.toString: no valid value returned for the route ' + location)
   return html.toString()
 }
@@ -206,9 +206,9 @@ Choo.prototype._matchRoute = function (location, queryString) {
   return this.state
 }

-Choo.prototype._prerender = function (state, emit) {
+function prerender (choo, state) {
   var routeTiming = nanotiming("choo.prerender('" + state.route + "')")
-  var res = state._handler(state, this.emit)
+  var res = state._handler(state, emit)
   routeTiming()
   return res
 }

marcbachmann · 2018-01-02T22:00:06Z

index.js

+
+Choo.prototype._matchRoute = function (location, queryString) {
+  if (location === undefined) location = this._createLocation()
+  if (queryString === undefined) queryString = window.location.search


note to myself: I shoud add a comment about that queryString is only used in the browser.
Or even move it up to the function call... depending how we'd like to handle it in #609

johanalkstal · 2018-01-13T10:57:21Z

I'm really eager to get this released! :D
Without this it's hard to redirect to other routes based on where the user is trying to go (prevent user from going to routes they should not have access to).

marcbachmann · 2018-01-15T14:07:28Z

Nanorouter just got published. So this could land soon in master
...after updating to nanorouter@3.0.0

- This fixes choojs#530 - Adds support for wayfarer/nanorouter subrouters

marcbachmann · 2018-01-15T19:08:38Z

@yoshuawuyts this is ready to merge.
We should also tackle some tests soon.

yoshuawuyts

rad!

yoshuawuyts · 2018-01-16T16:13:11Z

How should we publish this? Semver... minor, right?

bates64 · 2018-01-16T16:23:04Z

@yoshuawuyts I'd argue major - this fixes #530, which is definitely not a backwards-compatible patch, as it changes behaviour that some apps may rely on...

Although, choo 7 has it's own goals and releasing that early won't be fun, so minor could work.

marcbachmann · 2018-01-16T18:29:30Z

It could be a patch as it fixes only the state of the navigate event. The sub-route support is just a nice add-on.
Now because the state change happens earlier, this could break something in case somebody accesses some old state value in within the navigate event... just as @heyitsmeuralex mentioned.
But because most consumers saw that as bug, I'd do a patch or minor version.

So to sum up, there are the following changes in this PR:

Fixes the routing to modify the state before the navigate event happens. This means you can access the updated state within the navigate event. Modifies the state before the render event is triggered. Therefore this fixes State out of date when switching routes #530, Fire route event after render if the url has changed #553, state.query not set until navigate #549, Navigate event should enable action on "page entry" #610
Exposes .emit as an alias to .emitter.emit
Theoretically supports nested routes 😁 . But because we didn't document nor test the router in this repo, I'd handle that as private api for now.

espy · 2018-01-17T17:31:40Z

Sounds like it would also fix #463 😸

yoshuawuyts · 2018-01-17T22:58:53Z

Cool, going to release as minor then! - Seems reasonable given the conversation. It's always this weird situation where one person's bug is another person's feature.

marcbachmann · 2018-01-17T23:00:25Z

❤️ Thanks 🎉

yoshuawuyts · 2018-01-17T23:01:22Z

choo@6.7.0 🎉

marcbachmann · 2018-01-17T23:23:36Z

For future reference and people who still want to access the old state within the navigate method,
please use choo.emitter.prependListener('navigate', func) instead.

marcbachmann commented Dec 27, 2017

View reviewed changes

marcbachmann mentioned this pull request Dec 27, 2017

Add .match method which returns {handler, params, route} choojs/nanorouter#8

Closed

yoshuawuyts approved these changes Dec 27, 2017

View reviewed changes

marcbachmann mentioned this pull request Dec 28, 2017

Add .match(route) to get a matching route without invoking the handler choojs/wayfarer#64

Merged

marcbachmann commented Dec 30, 2017

View reviewed changes

marcbachmann force-pushed the separate-route-matching-from-rendering branch 2 times, most recently from 993867a to 049d57e Compare January 2, 2018 16:26

marcbachmann commented Jan 2, 2018

View reviewed changes

Separate route matching from rendering

9aa1015

- This fixes choojs#530 - Adds support for wayfarer/nanorouter subrouters

marcbachmann force-pushed the separate-route-matching-from-rendering branch from 049d57e to 9aa1015 Compare January 15, 2018 18:42

yoshuawuyts approved these changes Jan 16, 2018

View reviewed changes

yoshuawuyts merged commit cf8c316 into choojs:master Jan 17, 2018

marcbachmann deleted the separate-route-matching-from-rendering branch January 17, 2018 23:00

marcbachmann mentioned this pull request Feb 15, 2018

Docs: Code to listen for 'navigation' change does not output written behavior #627

Open

goto-bus-stop mentioned this pull request Mar 5, 2018

move ._handler off state #637

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate route matching from rendering #613

Separate route matching from rendering #613

marcbachmann commented Dec 27, 2017 •

edited

Loading

marcbachmann Dec 27, 2017 •

edited by yoshuawuyts

Loading

yoshuawuyts Jan 2, 2018

marcbachmann Jan 2, 2018

marcbachmann Jan 2, 2018

yoshuawuyts left a comment

marcbachmann commented Dec 27, 2017

marcbachmann Dec 30, 2017

yoshuawuyts Jan 2, 2018

marcbachmann Jan 2, 2018

marcbachmann commented Jan 2, 2018 •

edited

Loading

marcbachmann Jan 2, 2018

johanalkstal commented Jan 13, 2018

marcbachmann commented Jan 15, 2018

marcbachmann commented Jan 15, 2018 •

edited

Loading

yoshuawuyts left a comment

yoshuawuyts commented Jan 16, 2018

bates64 commented Jan 16, 2018 •

edited

Loading

marcbachmann commented Jan 16, 2018

espy commented Jan 17, 2018

yoshuawuyts commented Jan 17, 2018 •

edited

Loading

marcbachmann commented Jan 17, 2018

yoshuawuyts commented Jan 17, 2018

marcbachmann commented Jan 17, 2018

Separate route matching from rendering #613

Separate route matching from rendering #613

Conversation

marcbachmann commented Dec 27, 2017 • edited Loading

marcbachmann Dec 27, 2017 • edited by yoshuawuyts Loading

Choose a reason for hiding this comment

yoshuawuyts Jan 2, 2018

Choose a reason for hiding this comment

marcbachmann Jan 2, 2018

Choose a reason for hiding this comment

marcbachmann Jan 2, 2018

Choose a reason for hiding this comment

yoshuawuyts left a comment

Choose a reason for hiding this comment

marcbachmann commented Dec 27, 2017

marcbachmann Dec 30, 2017

Choose a reason for hiding this comment

yoshuawuyts Jan 2, 2018

Choose a reason for hiding this comment

marcbachmann Jan 2, 2018

Choose a reason for hiding this comment

marcbachmann commented Jan 2, 2018 • edited Loading

marcbachmann Jan 2, 2018

Choose a reason for hiding this comment

johanalkstal commented Jan 13, 2018

marcbachmann commented Jan 15, 2018

marcbachmann commented Jan 15, 2018 • edited Loading

yoshuawuyts left a comment

Choose a reason for hiding this comment

yoshuawuyts commented Jan 16, 2018

bates64 commented Jan 16, 2018 • edited Loading

marcbachmann commented Jan 16, 2018

espy commented Jan 17, 2018

yoshuawuyts commented Jan 17, 2018 • edited Loading

marcbachmann commented Jan 17, 2018

yoshuawuyts commented Jan 17, 2018

marcbachmann commented Jan 17, 2018

marcbachmann commented Dec 27, 2017 •

edited

Loading

marcbachmann Dec 27, 2017 •

edited by yoshuawuyts

Loading

marcbachmann commented Jan 2, 2018 •

edited

Loading

marcbachmann commented Jan 15, 2018 •

edited

Loading

bates64 commented Jan 16, 2018 •

edited

Loading

yoshuawuyts commented Jan 17, 2018 •

edited

Loading