Skip to content

No longer recreating histories within MemoryRouter #151

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 50 additions & 34 deletions src/controllers/memory-router/index.tsx
Original file line number Diff line number Diff line change
@@ -1,57 +1,73 @@
import React from 'react';
import React, { useRef } from 'react';

import { createMemoryHistory, MemoryHistoryBuildOptions } from 'history';
import { createMemoryHistory } from 'history';

import { Router } from '../router';
import { RouterProps } from '../router/types';

import { MemoryRouterProps } from '../../common/types';

const getRouterProps = (memoryRouterProps: MemoryRouterProps) => {
const {
isStatic = false,
isGlobal = true,
basePath,
routes,
resourceData,
resourceContext,
} = memoryRouterProps;
let routerProps: Partial<RouterProps> = {
basePath,
routes,
isStatic,
isGlobal,
const lazy = <T extends any>(callback: () => T) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let firstCall = true;
let current: T | undefined = undefined;

return () => {
if (firstCall) {
current = callback();
firstCall = false;
}

return current;
};
};

if (resourceData) {
routerProps = { ...routerProps, resourceData };
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
const useMemoryHistory = (location: string | undefined) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stored all the ugliness of trying to retain the same instance across renders in this hook.

const newGetHistory = lazy(() =>
createMemoryHistory({
initialEntries: location !== undefined ? [location] : undefined,
})
);

if (resourceContext) {
routerProps = { ...routerProps, resourceContext };
const historyStateCandidate = {
getHistory: newGetHistory,
location,
};

const historyState = useRef(historyStateCandidate);

if (historyState.current.location !== historyStateCandidate.location) {
historyState.current = historyStateCandidate;
}

return routerProps;
return historyState.current.getHistory();
};

/**
* Ensures the router store uses memory history.
*
*/
export const MemoryRouter = (props: MemoryRouterProps) => {
const { location, children } = props;
const config: MemoryHistoryBuildOptions = {};

if (location) {
config.initialEntries = [location];
}

const history = createMemoryHistory(config);
const routerProps = getRouterProps(props);
export const MemoryRouter = ({
isStatic = false,
isGlobal = true,
location,
children,
basePath,
routes,
resourceData,
resourceContext,
}: MemoryRouterProps) => {
const history = useMemoryHistory(location);

return (
// @ts-ignore suppress history will be overwritten warning
<Router history={history} {...(routerProps as RouterProps)}>
<Router
isStatic={isStatic}
isGlobal={isGlobal}
history={history}
basePath={basePath}
routes={routes}
resourceData={resourceData}
resourceContext={resourceContext}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

>
{children}
</Router>
);
Expand Down