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

Provide component middleware helper and documentation #3316

Closed
ryanflorence opened this issue Apr 14, 2016 · 12 comments
Closed

Provide component middleware helper and documentation #3316

ryanflorence opened this issue Apr 14, 2016 · 12 comments
Labels

Comments

@ryanflorence
Copy link
Member

ryanflorence commented Apr 14, 2016

I cooked this up a little bit ago and I really like it:

https://github.com/ryanflorence/react-router-apply-middleware

Router middleware can be really powerful, but it's tricky to build it in a way that makes it composable with other middleware because you have to compose both the render prop and the createElement prop, for example, to compose just three middleware together it would look like this:

<Router
  render={(props) => (
    <AsyncPropsRootContainer {...props}
      render={(props) => (
        <NamedRoutesRootContainer {...props}
          render={(props) => (
            <RelativeLinksRootContainer {...props}
              createElement={(Component, props) => (
                <AsyncPropsContainer Component={Component} routerProps={props} loadContext={token}
                  createElement={(Component, props) => (
                    <RelativeLinksContainer Component={Component} routerProps={props}/>
                  )}
                />
              )}
            />
          )}
        />
      )}
    />
  )}
/>

All the renders go first, then all the createElements go into the last render in the chain, and each has to work as the first, last, or middle in the chain. And so you have to hope that the authors of all three did everything right w/ their own render and createElement props. They'd all have to do the exact same thing.

This API looks a little crazy when used like this, but I don't think that means the router has a bad extension API--I think it's solid--but we can definitely clean up the front-door to it.

applyRouterMiddleware makes middleware composable by default and much simpler for both the authors and consumers:

Consumers have way less to do now:

const render = applyMiddleware(
  useAsyncProps({ loadContext: { token } }),
  useNamedRoutes(),
  useRelativeLinks()
)
<Router render={render}/>

It's also pretty easy for middleware authors. A middleware is just a function that provides an object with two render methods, one for render and one for createElement.

const useFoo = () => ({
  // same signature as Router.props.render
  renderRootContainer: (renderProps) => (
    <FooRootContainer {...renderProps}/>
  ),

  // same signature as Router.props.createElement
  renderContainer: (Component, props) => (  
    <FooContainer Component={Component} routerProps={props}/>
  )
})

Then applyMiddleware takes all of those elements and creates that mess from earlier. Authors no longer have to provide render and createElement props or even think about how to compose w/ others, they just have to worry about what their components do.

I'm undecided on the method names. I picked renderRootContainer and renderContainer because when I author middleware I usually use a RootContainer in render and a Container in createElement, maybe they should be render and createElement so they map exactly to the Router's names for these methods.

I'd like to bring this into core and then document how to author middleware so folks can add whatever middleware they need and have it compose with everybody else's.

This is a follow up from #2376

@ryanflorence
Copy link
Member Author

If I can get a +1 from somebody else I'll work up a PR

@mjackson
Copy link
Member

Seems like providing both render and createElement is overkill. Can't we
just give people a render method?
On Thu, Apr 14, 2016 at 4:22 PM Ryan Florence notifications@github.com
wrote:

I cooked this up a little bit ago and I really like it:

https://github.com/ryanflorence/react-router-apply-middleware

Router middleware can be really powerful, but it's tricky to build it in a
way that makes it composable with other middleware because you have to
compose both the render prop and the createElement prop, for example, to
compose just three middleware together it would look like this:

<Router
render={(props) => (
<AsyncPropsRootContainer {...props}
render={(props) => (
<NamedRoutesRootContainer {...props}
render={(props) => (
<RelativeLinksRootContainer {...props}
createElement={(Component, props) => (
<AsyncPropsContainer Component={Component} routerProps={props} loadContext={token}
createElement={(Component, props) => (

)}
/>
)}
/>
)}
/>
)}
/>
)}
/>

And you have to hope that the authors of all three did everything right w/
their own render and createElement props, which can be a bit tricky too,
where both need to compose as a middleware or be the last in the chain and
render a for render or the for createElement.
Every middleware would need to do the exact same thing.

applyRouterMiddleware makes middleware composable by default and much
simpler for both the authors and consumers:

Consumers have way less to do now:

const render = applyMiddleware(
useAsyncProps({ loadContext: { token } }),
useNamedRoutes(),
useRelativeLinks()
)

It's also pretty easy for middleware authors. A middleware is just a
function that provides an object with two render methods, one for render
and one for createElement.

const useFoo = () => ({
// same signature as Router.props.render
renderRootContainer: (renderProps) => (
<FooRootContainer {...renderProps}/>
),

// same signature as Router.props.createElement
renderContainer: (Component, props) => (

)
})

I'm undecided on the method names. I picked renderRootContainer and
renderContainer because when I author middleware I usually use a
RootContainer in render and a Container in createElement, maybe they
should be render and createElement so they map exactly to the Router's
names for these methods.

I'd like to bring this into core and then document how to author
middleware so folks can add whatever middleware they need and have it
compose with everybody else's.

This is a follow up from #2376
#2376


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#3316

@ryanflorence
Copy link
Member Author

ryanflorence commented Apr 14, 2016

You need both, the root container provides some context that the container passes to route components. For example, in AsyncProps, the root container keeps the data in state, puts it on context, and then the Container pulls it from context and passes it as props to the route component.

Relay router also needs both.

The router renders in two places, the top level Router and the components inside of RouterContext, and so middleware needs to hop in at those two places also.

@taion
Copy link
Contributor

taion commented Apr 14, 2016

I'm 👍 on this change, specifically with the two render hooks. I think the createElement hook is what makes the middleware concept useful.

If you take a look at what I have in react-router-relay, it's a bit fiddly. I need to both set the default value for createElement at https://github.com/relay-tools/react-router-relay/blob/v0.11.0/src/RelayRouterContext.js#L22 and then make sure I'm calling into this.props.createElement in https://github.com/relay-tools/react-router-relay/blob/v0.11.0/src/RelayRouterContext.js#L46-L66. If I don't do this, then this wrapper won't compose properly.

There's also another problem with this approach of wrapping createElement. If you define things like:

<FooRootContainer>
  <BarRootContainer>
    <RouterContext />
  </BarRootContainer>
</FooRootContainer>

Because of the way the wrapped createElement propagates down, when rendering route handlers, you get:

<BarRouteContainer>
  <FooRouteContainer>
    <RouteComponent />
  </FooRouteContainer>
</BarRouteContainer>

This might not be a problem in practice, but it's a bit confusing and awkward.

@ryanflorence
Copy link
Member Author

ryanflorence commented Apr 14, 2016

To implement relative routes, I only needed createElement, to put the route on context for Links while other middleware would only need to use render, and then any data loading middleware (relay, falcor, asyncprops) will need both so it can 1) hijack location changes to load data, pause the world, etc. for the entire render tree and 2) pass that data as props to specific route components.

Again, the router takes control of rendering in two places, so middleware needs to be able to participate in both places.

@ryanflorence ryanflorence added this to the next-2.4.0 milestone Apr 14, 2016
@timdorr
Copy link
Member

timdorr commented Apr 15, 2016

To implement relative routes, I only needed createElement, to put the route on context for Links

Any reason we don't do this already? We had talked about doing this in the past to build up support for relative Links being a core feature at some point.

@ryanflorence
Copy link
Member Author

ryanflorence commented Apr 15, 2016

@timdorr #2172 (comment)

I like trying to enhance the router from the outside to make sure we have a decent API for extending. We'll bring it into core though when it's working well enough.

@ryanflorence
Copy link
Member Author

#3325

timdorr pushed a commit that referenced this issue Apr 15, 2016
@ryanflorence
Copy link
Member Author

follow #3327

taion pushed a commit that referenced this issue Apr 16, 2016
@perrin4869
Copy link
Contributor

Is this usable in an isomorphic app?

@taion
Copy link
Contributor

taion commented Apr 19, 2016

@perrin4869

The result of applyRouterMiddleware should be render-able as something with the same API as <RouterContext>, so you should be able to do something like:

const RouterContext = applyRouterMiddleware(middlewares)
// Then that's your <RouterContext> to render with!

@perrin4869
Copy link
Contributor

@taion, I see, thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

5 participants