-
Notifications
You must be signed in to change notification settings - Fork 2
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
Simplify state handling #5
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,123 +1,67 @@ | ||
import React, { useState, useEffect, useRef } from 'react'; | ||
import React, { useState, useEffect, useRef, useCallback } from 'react'; | ||
import { createBrowserHistory, History } from 'history'; | ||
import { RouterContext } from '../Context'; | ||
|
||
import { Route } from '../types'; | ||
import { Route, Router as IRouter } from '../types'; | ||
import Path from '../PathUtils'; | ||
|
||
export interface RouterState { | ||
location: string; | ||
params: Object; | ||
routes: Array<Route>; | ||
context: Object; | ||
forceRefresh: number; | ||
routedElement: React.ReactNode | undefined; | ||
} | ||
|
||
export interface IRouterProps { | ||
routes: Array<Route>; | ||
children: React.ReactNode | ||
} | ||
|
||
export const Router: React.FC<IRouterProps> = props => { | ||
const history = useRef<History | undefined>(undefined); | ||
const initialState: RouterState = { | ||
location: '', | ||
params: {}, | ||
context: {}, | ||
routes: props.routes, | ||
routedElement: undefined, | ||
forceRefresh: 0 | ||
}; | ||
const browserHistory = createBrowserHistory() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This now lives in a constant since the only reason for it being a ref was because it lived inside the component so it needed to mutate once. Now it's a singleton for the life of the module. |
||
|
||
const [state, setState] = useState(initialState); | ||
const getComponentFromRoute = (route: Route, router: IRouter) => { | ||
let guardReturn: React.ReactNode = undefined; | ||
|
||
const setLocation = (location: string) => { | ||
if (state.location !== location) { | ||
history.current?.push(location); | ||
} else { | ||
setState({ | ||
...state, | ||
forceRefresh: state.forceRefresh + 1 | ||
}); | ||
if (route.guards) { | ||
let nexted = false; | ||
for (const guard of route.guards) { | ||
guardReturn = guard(router, () => { | ||
nexted = true | ||
return undefined | ||
}) | ||
if (!nexted && guardReturn) break | ||
} | ||
}; | ||
|
||
const setContext = (context: Object) => { | ||
setState({ | ||
...state, | ||
context: Object.assign(state.context, context) | ||
}); | ||
}; | ||
} | ||
|
||
useEffect(() => { | ||
history.current = createBrowserHistory(); | ||
setState({ | ||
...state, | ||
location: history.current.location.pathname | ||
}); | ||
|
||
const unlisten = history.current.listen(update => { | ||
setState({ | ||
...state, | ||
location: update.location.pathname | ||
}); | ||
}); | ||
return guardReturn ?? route.component | ||
} | ||
|
||
return () => { | ||
unlisten(); | ||
}; | ||
}, []); | ||
export const Router = ({ routes, children }: IRouterProps) => { | ||
const [location, setLocation] = useState<string>(browserHistory.location.pathname) | ||
const [context, setContext] = useState<object>({}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I minimized the state necessary to run the router. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice! With this every change in context will trigger the router logic. The only concern I have is if it could be useful to change the router context without triggering a reroute? |
||
|
||
// Listen to location changes | ||
useEffect(() => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This effect is now the |
||
let route = state.routes.find(route => { | ||
return Path.match(route.path, state.location); | ||
browserHistory.listen(update => { | ||
setLocation(update.location.pathname); | ||
}); | ||
}, []); | ||
|
||
if (route) { | ||
let guardReturn: React.ReactNode = undefined; | ||
let nexted = false; | ||
|
||
if (route.guards) { | ||
for (const guard of route.guards) { | ||
guardReturn = guard({ | ||
location: state.location, | ||
setLocation, | ||
setContext, | ||
context: state.context, | ||
params: state.params, | ||
}, () => { | ||
nexted = true | ||
return undefined | ||
}); | ||
if (!nexted && guardReturn) break; | ||
} | ||
} | ||
|
||
setState({ | ||
...state, | ||
params: Path.parse(route.path, state.location), | ||
routedElement: guardReturn ?? route.component | ||
}); | ||
|
||
} | ||
|
||
const route = routes.find(route => Path.match(route.path, location)); | ||
|
||
if (!route) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will help catch bugs in client applications. |
||
console.error(`Current location ${location} did not match any defined route`) | ||
return null; | ||
} | ||
|
||
}, [state.location, state.forceRefresh]); | ||
const params = Path.parse(route.path, location) | ||
const component = getComponentFromRoute(route, { location, setLocation, context, setContext, params }) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Both this and the route computation could be memoized but I think every re-render will need to recompute it anyway so I think it's safe to just roll with it. On very heavy routings (hundreds of thousands of routes times hundreds of thousands of guards per route, this could be slow) but if the case ever comes up we could just optimize it. |
||
|
||
return ( | ||
<RouterContext.Provider | ||
value={{ | ||
location: state.location, | ||
context: state.context, | ||
setLocation: setLocation, | ||
setContext: setContext, | ||
params: state.params, | ||
component: state.routedElement | ||
params, | ||
location, | ||
setLocation, | ||
context, | ||
setContext, | ||
component | ||
}} | ||
> | ||
{props.children} | ||
{children} | ||
</RouterContext.Provider> | ||
); | ||
}; | ||
) | ||
} |
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.
No longer necessary to do manual refresh, since we are now fully reactive to changes in context.