No longer recreating histories within MemoryRouter #151
Conversation
PatrickShaw
force-pushed
the
avoid-create-memory-history-in-memory-router
branch
3 times, most recently
from
e026d73 to
7151009
Compare
PatrickShaw
force-pushed
the
avoid-create-memory-history-in-memory-router
branch
from
7151009 to
4a2f26d
Compare
| return ( | ||
| // @ts-ignore suppress history will be overwritten warning | ||
| <Router history={history} {...(routerProps as RouterProps)}> | ||
| <Router history={historyState.current.getHistory()} {...other}> |
There was a problem hiding this comment.
getHistory will retrieve the same history instance so longer as location doesn't change.
There was a problem hiding this comment.
as RouterProps was removed as it makes tsc complain that the history prop is always overridden (since RouterProps always defined history). I'm surprised this wasn't previously causing TS compilation to fail.
Possibly my node_modules versions differ from what this code was originally tested on. Still, I think it's an improvement.
| }; | ||
|
|
||
| if (resourceData) { | ||
| routerProps = { ...routerProps, resourceData }; |
There was a problem hiding this comment.
I'm not sure what the intention was here. Router allows resourceData and resourceContext to be defined so I've removed these if conditions in the equivalent destructure.
| basePath={basePath} | ||
| routes={routes} | ||
| resourceData={resourceData} | ||
| resourceContext={resourceContext} |
There was a problem hiding this comment.
Normally I'd just spread the props from MemoryRouter onto Router but it looks like the code intentionally picked out specific props using getRouterProps so I've retained equivalent logic in this PR.
| const newGetHistory = () => | ||
| createMemoryHistory({ | ||
| initialEntries: location !== undefined ? [location] : undefined, | ||
| }); |
There was a problem hiding this comment.
We don't want execute createMemoryHistory in the render function if it's not going to be used. Hence we store a reference to a callback rather than the history instance itself.
| createMemoryHistory({ | ||
| initialEntries: location !== undefined ? [location] : undefined, | ||
| }) | ||
| ); |
There was a problem hiding this comment.
We don't want to create an unnecessary history object on every render if we're not going to use it, so I've stored the history in a memoized callback instead (with the assumption that an uncalled callback is probably going to be cheaper than creating the history API).
| if (resourceData) { | ||
| routerProps = { ...routerProps, resourceData }; | ||
| } | ||
| const lazy = <T extends any>(callback: () => T) => { |
There was a problem hiding this comment.
Could use https://lodash.com/docs/4.17.15#memoize here. I see lodash being used elsewhere in this code base but it seemed a bit excessive since we don't need to do any argument caching.
| if (resourceData) { | ||
| routerProps = { ...routerProps, resourceData }; | ||
| } | ||
| const useMemoryHistory = (location: string | undefined) => { |
There was a problem hiding this comment.
Stored all the ugliness of trying to retain the same instance across renders in this hook.
Similar to #150 I don't think we want to create a new memory history on every render of
MemoryRouter. Memory history is supposed to maintain state outside the lifetime of a single render.I've fixed this issue by putting
useRefaround thecreateMemoryHistoryto retain the same instance.