Skip to content

Conversation

taion
Copy link
Contributor

@taion taion commented Oct 26, 2015

Closes #2088

Another attempt at #2376. The tests illustrate the intended usage. I think this is a much less gross API on the React Router side.

@taion
Copy link
Contributor Author

taion commented Oct 26, 2015

My intention here on the react-router-relay side then is to expose:

  • RelayRoutingWrapper
  • RelayRoutingContext
  • RelayRouter

The code would be something like:

const RelayRoutingContext = ({forceFetch, ...props}) => (
  <RelayRoutingWrapper {...props} forceFetch={forceFetch}>
    <RoutingContext {...props} />
  </RelayRoutingWrapper>
) 

const RelayRouter = (props) => (
  <Router {...props} RoutingContext={RelayRoutingWrapper} />
);

The idea is that most users can just use RelayRouter and RelayRoutingContext, while somebody who needs to can build their own custom RoutingContext with RelayRoutingWrapper and whatever other wrappers they need.

@taion
Copy link
Contributor Author

taion commented Oct 26, 2015

Done making trivial tweaks to the new test cases... finally.

Copy link
Member

Choose a reason for hiding this comment

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

(nit) This should be camelCased as routingContext.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Works for me - I've seen people do both for custom component classes depending on the context and don't have a personal preference.

Copy link
Member

Choose a reason for hiding this comment

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

I suppose it would be different if the DI here was done via module. But then you wouldn't even have this problem in the first place (you'd have others, but that's a different story...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implicitly I'm sorta re-using the module name here: https://github.com/rackt/react-router/pull/2383/files#diff-1063d4db6b1a5033d4afb74421a91ef4R31

It's not extremely clever, but it saves a tiny amount of typing.

@taion
Copy link
Contributor Author

taion commented Oct 27, 2015

@timdorr I don't mind changing this to routingContext. But since you're looking at this - does this make sense as an API entry point? This is pretty much resurrecting #1870, which we abandoned at the time but I think I need it at this point.

@taion
Copy link
Contributor Author

taion commented Oct 27, 2015

@timdorr Updated with your feedback.

I also realized that I have to pass extra props from Router into the custom routing context for things to work nicely, and I think it's reasonable to drop routes as a prop from route components with this PR.

@taion taion modified the milestone: v1.0.0-rc4 release Oct 27, 2015
@timdorr
Copy link
Member

timdorr commented Oct 28, 2015

Could you not communicate that information via context? Or could you not do what redux-router does and provide your own RelayRouter top-level component?

@taion
Copy link
Contributor Author

taion commented Oct 28, 2015

@timdorr I can - however, the only thing I need to override really is just the RoutingContext used. I could copy/paste the entirety of Router, but it seemed like it'd be cleaner if I could just implement RelayRouter as <Router {...this.props} routingContext={RelayRoutingContext} />.

@taion taion added this to the v1.1 milestone Oct 28, 2015
@taion
Copy link
Contributor Author

taion commented Oct 28, 2015

@mjackson @ryanflorence

If either of you have a chance, I'd appreciate it if you could have a quick look at the proposed API (i.e. adding a routerContext prop to Router) and let me know if this is a reasonable approach, or if I need to go back to the drawing board again.

This isn't relevant for 1.0.0, but it'd be helpful to know if this approach seems reasonable.

@taion
Copy link
Contributor Author

taion commented Nov 2, 2015

Shoot, if we want to drop routes as a prop from route component, this actually can't be for 1.1.

@ryanflorence
Copy link
Member

Keep this.props.routes

And then lets merge this.

@taion taion changed the title [wip][added] Support custom RoutingContext on Router [added] Support custom RoutingContext on Router Nov 3, 2015
@taion
Copy link
Contributor Author

taion commented Nov 3, 2015

Squashed, rebased, renamed routingContext back to RoutingContext. The test coverage here exercises everything I care about.

ryanflorence added a commit that referenced this pull request Nov 3, 2015
[added] Support custom RoutingContext on Router
@ryanflorence ryanflorence merged commit 7c129f5 into remix-run:master Nov 3, 2015
@taion taion deleted the custom-RoutingContext branch November 3, 2015 06:13
@lock lock bot locked as resolved and limited conversation to collaborators Jan 20, 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.

4 participants