-
-
Notifications
You must be signed in to change notification settings - Fork 6
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
Add route
to Router
type and abstract router logic into separate utilities
#37
Conversation
const replace: RouterReplace = (url) => { | ||
return push(url, { replace: true }) | ||
} | ||
|
||
const forward: Router<T>['forward'] = () => { | ||
throw 'not implemented' | ||
const forward: RouterNavigation = (number = 1) => { | ||
return go(number) | ||
} | ||
|
||
const back: Router<T>['forward'] = () => { | ||
throw 'not implemented' | ||
const back: RouterNavigation = (number = -1) => { | ||
return go(number) | ||
} | ||
|
||
const go: Router<T>['go'] = (_number) => { | ||
const go: RouterNavigation = (_number) => { | ||
throw 'not implemented' | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are actually final implementations I believe 🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved this into its own utility with a dedicated type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved this into its own utility and just passing in the routes. This makes this much simpler to test and also moves logic out of the createRouter
utility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
things are getting fun!
forward: () => void, | ||
go: (number: number) => void, | ||
routeMatch: (path: string) => Resolved<TRoutes[number]> | undefined, | ||
route: Readonly<Resolved<Route>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this the "current route"? I expected that to be possibly undefined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it's the current route. Which should never be undefined? When the app loads there should be a route right?
Unless there's a rejection 🤔 is that what you're thinking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess my thinking was more in error states, but I don't know that it's worth having to check for undefined everywhere in the event that the user might be in one of those "computed routes" where something like not-found, or needs-auth is controlling the route. I didn't know if those were still going to be legit "routes"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a great question that I hadn't thought of. I don't know the answer. Will have to think about that.
No description provided.