-
Notifications
You must be signed in to change notification settings - Fork 28
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
fix withRouter types #24
base: master
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,14 @@ | ||
import React, { ComponentType } from 'react'; | ||
|
||
import { BrowserHistory, RouteContext } from '../../common/types'; | ||
import { RouterActionPush, RouterActionReplace } from '../router-store/types'; | ||
import { RouterSubscriber } from '../subscribers/route'; | ||
|
||
// TODO | ||
type WithRouter = RouteContext & { history: BrowserHistory }; | ||
export type WithRouterProps = RouteContext & { | ||
history: BrowserHistory; | ||
push: RouterActionPush; | ||
replace: RouterActionReplace; | ||
}; | ||
|
||
const getWrappedComponentDisplayName = ( | ||
component: ComponentType<any> | ||
|
@@ -23,20 +27,20 @@ const getWrappedComponentDisplayName = ( | |
return `withRouter(${componentDisplayName})`; | ||
}; | ||
|
||
export const withRouter = <P extends Record<string, any>>( | ||
export const withRouter = <P extends Record<string, any> = {}>( | ||
WrappedComponent: ComponentType<P> | ||
) => { | ||
): ComponentType<Omit<P, keyof WithRouterProps>> => { | ||
const displayName = getWrappedComponentDisplayName(WrappedComponent); | ||
const Component = WrappedComponent as ComponentType<WithRouter & P>; | ||
const ComponentWithRouter = (props: P) => ( | ||
const Component = WrappedComponent; | ||
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. /nit May as well just renamed the |
||
const ComponentWithRouter = (unknownProps: any) => ( | ||
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. is "unknown" necessary? 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. no it's not |
||
<RouterSubscriber> | ||
{( | ||
// @ts-ignore access private `history` store property | ||
{ route, location, query, match, action, history }, | ||
{ push, replace } | ||
) => ( | ||
<Component | ||
{...props} | ||
{...unknownProps} | ||
route={route} | ||
location={location} | ||
query={query} | ||
|
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.
Could this just be made into an optional generic like in the
withRouter
case? This should only need to be supported for consumers that do not use the exportedRouteComponent
component, and I would avoid the use ofany
here as a result. The default prop type just just beRouteComponentProps
for those using the route component.