Skip to content
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

Allow a router prop to take priority over context prop. #3729

Merged
merged 1 commit into from
Aug 16, 2016

Conversation

elyobo
Copy link
Contributor

@elyobo elyobo commented Aug 16, 2016

withRouter should allow a router prop specified on a wrapped component
to be used instead of the router in context; this pattern is more
flexible, avoids overwriting explicitly set props, and makes testing
components wrapped by withRouter more simple.

withRouter should allow a router prop specified on a wrapped component
to be used instead of the router in context; this pattern is more
flexible, avoids overwriting explicitly set tests, and makes testing
components wrapped by withRouter more simple.
@taion
Copy link
Contributor

taion commented Aug 16, 2016

Does the React Redux @connect do this?

@elyobo
Copy link
Contributor Author

elyobo commented Aug 16, 2016

Yes, see here.

@taion
Copy link
Contributor

taion commented Aug 16, 2016

@elyobo
Copy link
Contributor Author

elyobo commented Aug 16, 2016

I'm not sure what you mean @taion; I thought your question was "does react redux allow passing a prop to take priority over context" and it's pretty clear here that it does...

@taion
Copy link
Contributor

taion commented Aug 16, 2016

For the store though... if you look at the version on next, the other connected props are not overridden by things passed down via props.

Well, I guess we can source them all from the same router.

@elyobo
Copy link
Contributor Author

elyobo commented Aug 16, 2016

The defaultMergeProps that you link to doesn't overwrite props with context though, which is what the current withRouter implementation does; this is the behaviour I'm looking to change. Whatever the result of a router prop after defaultMergeProps is, withRouter would currently overwrite it.

@elyobo
Copy link
Contributor Author

elyobo commented Aug 16, 2016

I see what you mean about the priority for defaultMergeProps; you're right that props specified on the component will potentially overwritten by props generated from the state or dispatch mapping. I think that the behaviour I'm introducing here is the same as that of the HOC that connect generates, in that a specified prop (from any of those sources) will take priority over the context.

That said, this is potentially a BC problem (if someone is currently specifying a router prop it's being ignored; that won't be the case after this change) and it's ultimately easy to roll my own withRouter and use that instead if I want this behaviour, so feel free to close if you're happier with it the way it is :)

@taion
Copy link
Contributor

taion commented Aug 16, 2016

I think this is fine. We'll need to separately backport (forwardport?) to the next branch but that should be easy.

LGTM.

@timdorr
Copy link
Member

timdorr commented Aug 16, 2016

Thanks!

@timdorr timdorr merged commit ab61a13 into remix-run:master Aug 16, 2016
@elyobo elyobo deleted the allow-router-as-prop branch August 16, 2016 23:48
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants