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

Replace "path" with "location" for SSR + React Router. #514

Merged
merged 1 commit into from Mar 3, 2018

Conversation

Projects
None yet
4 participants
@ChadBurggraf
Contributor

ChadBurggraf commented Mar 1, 2018

According to the documentation, as well as my testing,
React Router expects the URL supplied to StaticRouter
to be provided via the location prop name.

React.NET currently supplies this value via the
path prop name.

Replace "path" with "location" for SSR + React Router.
According to the documentation, as well as my testing,
React Router expects the URL supplied to `StaticRouter`
to be provided via the `location` prop name.

React.NET currently supplies this value via the
`path` prop name.
@Daniel15

This comment has been minimized.

Member

Daniel15 commented Mar 3, 2018

@gunnim or @dustinsoftware could you please take a look at this? I don't have much experience with ReactRouter. How was this working before? Did it change in a newer Router version?

@dustinsoftware dustinsoftware self-assigned this Mar 3, 2018

@dustinsoftware

This comment has been minimized.

Collaborator

dustinsoftware commented Mar 3, 2018

This change makes the React.NET props more consistent with React Router. Previous use in our sample:

<StaticRouter context={this.props.context} location={this.props.path}>

It is a trivial change for any current consumers of React.Router, and it's worth merging for better consistency.

@dustinsoftware dustinsoftware merged commit 5dd6dd3 into reactjs:master Mar 3, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@gunnim

This comment has been minimized.

Contributor

gunnim commented Mar 5, 2018

The idea is that you always wrap the StaticRouter, allowing you to render children and target the props shown in the example by dustinsoftware
My original choice probably had to do with the fact that the location prop for the router also supports location objects which we are never passing down on the server side.

I think I agree this change would be more consistent but I think it's also trivial to continue to support those using the original implementation by simply passing down a location prop as well as the "deprecated" path prop.

@dustinsoftware

This comment has been minimized.

Collaborator

dustinsoftware commented Mar 6, 2018

Thanks gunnim. I think we should still stick with this change, passing down both may trigger prop-type warnings (such as forbidExtraProps), and migrating should be pretty painless. Since React Router support is pretty new the risk should be pretty low that someone will get blindsided by this change (it gets called out in the release notes)

@gunnim

This comment has been minimized.

Contributor

gunnim commented Mar 6, 2018

True, good points :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment