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

onDispatch callback and connect middleware #46

Open
wants to merge 14 commits into
base: 1.x
Choose a base branch
from

Conversation

matthewwithanm
Copy link
Collaborator

This PR has two additions, though the first is really only a means to the second.

Firstly, it adds an onDispatch callback. This is basically the same as onNavigation, but it's also called on componentWillMount.

The second, and main addition, is a connect middleware. It works like this:

var reactRouter = require('react-router-component/lib/contrib/connect-middleware');

// ...create app...

app.use(reactRouter(MyRouterComponent));

Your node app will now use RRC to route requests, sending the appropriate status code! (404 or 200)

By default, it adds an HTML doctype and serves the app as "text/html", however, this can be customized with a second argument:

app.use(reactRouter(MyRouterComponent, {doctype: 'svg'})); // Shortcut doctypes
app.use(reactRouter(MyRouterComponent, {doctype: '<!DOCTYPE HTML ...>'})); // Long doctypes
app.use(reactRouter(MyRouterComponent, {contentType: 'application/xhtml+xml'})); // Guess doctype based on content type

etc.

You can also pass props:

app.use(reactRouter(MyRouterComponent, {props: {someProp: 'hello'}}));

@andreypopp I'm not sure how this plays with asynchronous components. Do you think you could take a look at that?

Though it can happen post-merge IMO, I think we need to continue to make it easier to create router components. The 1.x branch has been heading that direction already, but it's not quite there yet. Here's what a minimal "App" looks like now:

var App = React.createClass({
  mixins: [RoutingEnvironmentMixin],
  renderRouter: function() {
    return <Router path={ this.getPath() }>...</Router>;
  },
  match: function(url) {
    return this.renderRouter().match(url);
  },
  render: function() {
    return (
      <html>
        <head>...</head>
        <body>{ this.renderRouter() }</body>
      </html>
    );
  }
});

A lot of this is boilerplate I really want to get rid of. (Technically, I guess there is a more minimal example if you're willing to make your real router your top-level component instead of putting it in the body.) Also, having to create the router in match…well, sucks.

Still, I'm super psyched about being able to bridge the gap between my server and React app so easily—and to have React driving the process!

@andreypopp
Copy link
Collaborator

This looks great.

I have a suggestion.

I think server side rendering shouldn't require top level component to accept path and onDispatch props. Instead we could run rendering in so-called "server context" which we will provide to routers via React's context mechanism.

That context could provide not only pathname but also querystring and cookies (basically info from req) so we can use it to render/route components based on such data (also available in browser).

As you can see there's already QuerystringKeyEnvironment which routes by a specified key in querystring. Currently it doesn't work on server because we can't read querystring there, but with "server context" we can.

We also can notify "server context" if there's "not found situation" happened in some router via a callback.

Note that this will require changing how environments instantiated, because we want them to read data from context instead of DOM. I think we can make all environments to have similar constructor signatures:

function PathnameEnvironment(context) {
  ...
}

function QuerystringKeyEnvironment(context, key) {
  ...
}

and pass to components not environment instances but some kind of environment factories which will be instantiated to environments in place with "server context" or "browser context" depending on what's available currently.

What do you think?

@matthewwithanm
Copy link
Collaborator Author

@andreypopp Updated to use ReactAsync for rendering—that was easier than I expected! Async rendering on the server side with the middleware is working!!!

@killercup
Copy link

How's the state on this? Has anything changed since April?

Or, what I'm actually interested in: When rendering in node, how can I determine the request is a 404 and set the correct status?

@STRML
Copy link
Owner

STRML commented Oct 18, 2014

I've taken over maintaining this module and I have yet to take a good look at this PR. I will need some time - thanks for bringing this to my attention.

@killercup
Copy link

Okay, I didn't know this project switched maintainers.

In the meantime I'll be using a workaround to set the HTTP status from within my isomorphic code. Basically, I'll just pass a setHTTPStatus function along as a prop when rendering on the server and it will be called in the NotFound handler. My code is similar to this:

// app.js
var React = require('react');
var {Locations, Location, NotFound} = require('react-router-component');
module.exports = React.createClass({
  render () {
    return Locations({path: this.props.path}, [
      NotFound({handler: require('./404'), setHTTPStatus: (this.props.setHTTPStatus || function() {})})
    ]);
  }
});
// 404.js
var React = require('react');
module.exports = React.createClass({
  render () {
    if (typeof this.props.setHTTPStatus === 'function') {
      this.props.setHTTPStatus(404);
    }
    return React.DOM.h1({}, 'not found');
  }
});
// server.js
var React = require('react');
var App = require('./app');

// ... express stuff ...
server.use(function(req, res, next) {
  var error, page;
  try {
    page = App({
      path: req.path,
      setHTTPStatus: function(code) {
        return res.status(code);
      }
    });
    return res.send(render({
      data: {
        html: React.renderComponentToString(page)
      }
    }));
  } catch (_error) {
    error = _error;
    return next(error);
  }
});
// ... express stuff ...

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

Successfully merging this pull request may close these issues.

4 participants