Updated Router Functionality#1
Conversation
kwalker3690
changed the title
kwalker3690
force-pushed
the
kelly/return-early-change-route
branch
from
608abee to
657746f
Compare
| @@ -85,62 +70,119 @@ export default class Router { | |||
| throw new Error('Router#registerRoutes must be passed an array of Routes') | |||
| } | |||
| routes.map(route => { | |||
There was a problem hiding this comment.
it's kind of silly this was a map before, probably more semantic to be a forEach
| if (this.onRouteComplete) { | ||
| const handlerLength = route.handlers.length-1; | ||
| const lastHandler = route.handlers[handlerLength]; | ||
| const routingEnd = (ctx, next) => { |
There was a problem hiding this comment.
This was super hacky before because page.js expects the last function to not have a next() function.
Instead it'd be nicer to push the config.onRouteComplete function if it is present and not have to hackily slice up the array of handler functions
| this.__routes = [] | ||
| this.__catchallPath = null; | ||
| this.__routes = []; | ||
| WindowEnv.removeEventListener('popstate', this.__onpopstate) |
There was a problem hiding this comment.
I wonder if reset should reset the route start and route end functions. what do you think?
There was a problem hiding this comment.
Yeah it definitely should, good call
| let i = 0; | ||
|
|
||
| let next = () => { | ||
| if (this.__currentCanonicalPath !== ctx.canonicalPath) { |
There was a problem hiding this comment.
Hmmm i'm a little concerned over this mechanism to escape the route.
what happens if you click a new route, then click back to the same route. I suppose it's synchronous so it will probably work.
One idea I had was to just keep track of a number / id that way any new dispatch will abondon the old dispatch instead of trying to use the canonical path as the identifier.
Ex
constructor() {
this.dispatchId = 1;
}
__dispatch() {
this.dispatchId++;
if (ctx.dispatchId !== this.dispatchId) {
return
}
}
There was a problem hiding this comment.
Oh i like that a lot, i tried a similar approach but wound up keeping track of more state than was necessary, this is super clean
kwalker3690
force-pushed
the
kelly/return-early-change-route
branch
from
657746f to
be27015
Compare
kwalker3690
force-pushed
the
kelly/return-early-change-route
branch
from
be27015 to
ce739c7
Compare
jordangarcia
left a comment
jordangarcia
left a comment
There was a problem hiding this comment.
It'd be nice to have the new functionality tested here, but i wont block on that.
LGTM
| this.__routes.push(new Route(route)) | ||
| routes.forEach(route => { | ||
| route = new Route(route); | ||
| if (this.onRouteComplete) { |
2 participants
@jordangarcia can you take a look at these changes?