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

Server rendering #57

Closed
simenbrekken opened this issue Jun 29, 2014 · 157 comments · May be fixed by hixio-mh/react-router#4
Closed

Server rendering #57

simenbrekken opened this issue Jun 29, 2014 · 157 comments · May be fixed by hixio-mh/react-router#4

Comments

@simenbrekken
Copy link
Contributor

Route.spec.js contains a commented out test for server rendering but it seems, server rendering would at least require exposing findMatches in the main module.

Any plans for reintroducing server rendering support?

@ryanflorence
Copy link
Member

Yes, absolutely. We have plans that will probably happen this week.

Sent from my iPhone

On Jun 29, 2014, at 12:57 PM, Simen Brekken notifications@github.com wrote:

Route.spec.js contains a commented out test for server rendering but it seems, server rendering would at least require exposing findMatches in the main module.

Any plans for reintroducing server rendering support?


Reply to this email directly or view it on GitHub.

@mjackson
Copy link
Member

There is a bunch of discussion in #38 that is relevant to server-side rendering. Let's continue that discussion here.

@phated
Copy link

phated commented Jul 2, 2014

Looking forward to this. Started digging through old issues and the code, but not sure where everyone is planning to start. Willing to help in any way and usually on IRC 👍

@th0r
Copy link

th0r commented Jul 16, 2014

It throws an error in Node during initialization:

/Volumes/Work/new-project/node_modules/react-nested-router/modules/stores/URLStore.js:6
  hash: (window.addEventListener) ? 'hashchange' : 'onhashchange',
         ^
ReferenceError: window is not defined
    at Object.<anonymous> (/Volumes/Work/new-project/node_modules/react-nested-router/modules/stores/URLStore.js:6:10)

It's predictible, because there is no global window there.

@rpflorence, could you please inform about the status of server-side routing support?
I think, that your routing lib is the most convenient and I want to build my "isomorphic" application using it, but unsupported server-side routing stops me...

@mjackson
Copy link
Member

The major blocker for server-side rendering at this point is coming up with a sane strategy for fetching data that works on both the client and the server.

Requirements are:

  • It must be asynchronous
  • It must support rendering the view before the data has arrived, for components that want to display "loading..." UI on the client
  • It should also support synchronously passing in data, for testing

Prior art/ideas:

@phated
Copy link

phated commented Jul 16, 2014

@mjackson ugh, react-async is a pile, don't make people use threads to fetch data.

@mjackson
Copy link
Member

@phated the fibers stuff is stupid, agreed. But I really like the simplicity of getInitialStateAsync. You don't need threads to pull it off.

@phated
Copy link

phated commented Jul 16, 2014

@mjackson I think the crux of async server rendering is that React.renderComponentToString is a sync operation so async things can't happen inside it, leading him to use threads. I think fetching async and passing the data into the component as initial props (sync) is the only way to do server rendering.

@mjackson
Copy link
Member

@phated But he has ReactAsync.renderComponentToStringWithAsyncState which takes a callback, so fibers shouldn't be needed.

Right now I'm thinking:

  • Route#getInitialAsyncState for components to fetch async state. If routes declare this they are asynchronous. If not, they're sync.
  • this.props.initialAsyncState as a way for test code to manually pass in async state.
  • Router.renderComponentToString Which is basically an async-aware version of React.renderComponentToString.
  • Possibly Route#componentWillReceiveAsyncState as a lifecycle hook for components that need to know when they're about to receive some state from getInitialAsyncState. This would only be needed if a component needs to setState in response to some asynchronous state. Not sure if this ever is a good idea, but seems like it could be useful.

If components need to display "loading..." UI, they can check for this.props.initialAsyncState or just check for this.state.someAsyncProp directly.

@phated
Copy link

phated commented Jul 16, 2014

Sounds like a good plan. I think I actually have a place in current code where I would need the Route#componentWillReceiveAsyncState lifecycle method.

@mjackson
Copy link
Member

@rpflorence @ericflo @petehunt Would love to get your thoughts here.

@ryanflorence
Copy link
Member

@mjackson yeah, that's pretty much exactly what I'm thinking too, minus the lifecycle hook, though that is compelling. I think we should do everything but that, start using it, and then see if we ever think "shoot, that lifecycle hook sure would be nice right now!"

@ryanflorence
Copy link
Member

@phated can you explain how you imagine using componentWillReceiveAsyncState?

@ericflo
Copy link

ericflo commented Jul 16, 2014

This all sounds great to me! Agree with @rpflorence that the lifecycle hook seems like an optional extra. Just to be clear, on the client side there'd be no need to switch the renderComponent call from React to Router, correct?

Also, personally I'd name it something other than renderComponentToString if it acts different from React's method of the same name, and I'd remove the word "initial" from getInitialAsyncState, but this bikeshed is yours to paint :)

@mjackson
Copy link
Member

@ericflo Yes. You'll still use React.renderComponent on the client. The async-aware equivalent is just for server-side.

@phated
Copy link

phated commented Jul 17, 2014

Probably should gist or PR this but I want to see if I am even on the same track as everyone's thinking

var App = React.createClass({
  statics: {
    getInitialAsyncState: function(cb){
      setTimeout(function(){
        cb(null, {
          initial: 'state'
        });
      }, 100);
    }
  }
});
function renderComponentToStringAsync(component, cb){
  var done = function(err, state){
    if(err){
      cb(err);
    }

    component.setState(state, function(){
      var html = React.renderComponentToString(component);
      cb(null, html);
    });
  };

  component.getInitialAsyncState(done)
}

@sophiebits
Copy link
Contributor

I think I'd vote for promises over callbacks if we can.

@phated
Copy link

phated commented Jul 17, 2014

@spicyj I would too, but usually there's more push back the other way, concept remains the same. I am unsure if I captured the thought behind getInitialAsyncState.

@mjackson
Copy link
Member

@phated yeah, that's the basic idea. I think getInitialAsyncState should be an instance method, similar to getInitialState but async. Is there a particular reason you made it a static method?

Also, I think React will complain if we try and call component.getInitialStateAsync directly on a descriptor.

@petehunt
Copy link
Contributor

getInitialAsyncState() needs to be a function of the route IMO, not of the props. Otherwise you need to actually render in order to know what to fetch which won't work in a sync environment.

@phated
Copy link

phated commented Jul 17, 2014

@mjackson I was confused about statics, descriptors, etc. I agree with @petehunt that it needs to be on a route, which would be a prop right?

@mjackson
Copy link
Member

you need to actually render in order to know what to fetch

Why is that a problem?

@petehunt
Copy link
Contributor

Because you may need to fetch something to know what to render, then you may need to fetch again to render again, etc etc

@petehunt
Copy link
Contributor

This is a fiddle I made a while ago which somewhat mirrors our internal stuff: http://jsfiddle.net/R88XR/

The gist of the idea is that data fetching may only be a function of the route so that data fetching is batched together and render is batched together (synchronously). So there's a static fetchData(route) -> Promise<data> method.

The data you fetch is accessed at this.state.data. If you render on the server, the data is fetched before rendering and passed to the component via an initialData prop (which is copied to this.state.data). If there is no initialData, the component itself fetches on mount.

@mjackson
Copy link
Member

@petehunt I see. Thanks for sharing the example.

This would be so much easier if React's getInitialState and renderComponentToString were both async..

@mjackson
Copy link
Member

The approach in that fiddle works well for that example, but how does it scale to many nested components? Do you have to pass in all data from the top?

@petehunt
Copy link
Contributor

So the way I'd do it is have this mixin on each handler (which I'd rename to "entry point" if I had my way, btw) and return promises instead of callbacks. Run all the promises in parallel and pass their results back in as initialData props.

If a child component needs to fetch data, it can provide its own fetchData() method and its owner can pass it an initialData prop.

@ryanflorence
Copy link
Member

side note: props.activeRoute and handler are two things I'm comfortable
bikeshedding some more.

On Thu, Jul 17, 2014 at 1:34 PM, Pete Hunt notifications@github.com wrote:

So the way I'd do it is have this mixin on each handler (which I'd rename
to "entry point" if I had my way, btw) and return promises instead of
callbacks. Run all the promises in parallel and pass their results back in
as initialData props.

If a child component needs to fetch data, it can provide its own
fetchData() method and its owner can pass it an initialData prop.


Reply to this email directly or view it on GitHub
#57 (comment)
.

@karlmikko
Copy link

@sbrekken good call!

Server rendering also needs to ensure that the <Link/> activeRoute state is maintained. Also ensuring that the server side doesn't mix the tracking of activeRoute with other concurrent pages being rendered.

@karlmikko
Copy link

@rpflorence I have been pondering the problem of all the Stores being singletons on the Server Side. Where concurrent requests are both rendering different pages. Being on a singleton this would cause both to change route. Obviously the stores will need to change to be instances.

I have an idea for how to enable components like <Link /> know what instance of the store to use.

https://gist.github.com/karlmikko/07dec76a7bed23f79bdb

The gist allows a component to look up the this._owner tree of React to find the <Routes /> instance it is a child of. Then get all the store instances from that <Routes /> instance.

This allows the <Routes /> instance to hold the reference to all the store instances. When the <Routes /> instance is destroyed / out of scope; all the stores and listeners should be freed; pending they aren't leaked elsewhere. If the store instances are kept inside react component instances that are children of the parent <Routes /> instance, it should be fine. (Please someone tell me if this is wrong)

@ryanflorence
Copy link
Member

@sbrekken

Router.renderRoutesToString(routes, url).then(null, function(failReason) {
  if (failReason.code === 500) {
    res.send(500);
  }
});

You simply fail a promise in your getInitialAsyncState hooks and send along the information that you care about to send a response.

@yurynix
Copy link

yurynix commented Aug 10, 2014

@rpflorence your example is grate for when page/application consist of a few components, but usually the case
is more complicated, with much more components, when application gets larger, I aspire that components would be more stand-alone and reusable.

In flux, one might have controller-views governing any significant section of the page - means single route, such as /blog/:postId might have many "controller views",
lets say one for the post itself and another one for the comments box.

If comments box is a stand alone component, I can just drop it anywhere, lets say in the /photos/:photoId,
however, with your suggested approach, I'll be forced to split the data fetching from each of the components and push it to the top level - the route handler, which makes the component harder to re-use.

I wish that would be a way, to render the whole hierarchy of async components, when all the hierarchy reports "ready", the html is generated with initial state.
The easiest way to currently achieve that with react as far as I can see is make it render on the server "the client way", with React.renderComponent(), as I described previously.

It would be better if the "server rendering" would return a handle that could be rendered to html in a latter time, but I have no idea how to achieve that.

@ryanflorence
Copy link
Member

@yurynix It is a common idiom both on the server and the client to deserialize a url to a set of data for rendering. In traditional server frameworks you don't want your partial views hitting the database, you want to fetch data in expected places. It is no different here.

As for sharing components, send the data in as a property to the <CommentList/>. Any controller-views using it passes it the data.

We're all pretty settled on this api. Feel free to propose a new one.

@KyleAMathews
Copy link
Contributor

@yurynix I agree that it'd be great to be able to load data all the way down the component chain but that's... hard. At least as React is built atm as it expects rendering to be synchronous.

@simenbrekken
Copy link
Contributor Author

@rpflorence Sending 500 is fine if that's what you want, but in most cases you probably want to render something in the same way you render a <NotFound />, but with error-details. Right now I'm doing the following:

fetchDataPromise.then(function(data) {
  return {
    metadata: view.getMetadata ? view.getMetadata(data) : {},
    markup: React.renderComponentToString(Application({view: view(data)})),
    initialData: JSON.stringify(data)    
  };
}, function(error) {
  return {
    markup: React.renderComponentToString(Application({error: error}))
  };
}).then(function(locals) {
  res.send(template(locals));
});

It would be nicer being able to Router.renderRoutesToString(routes, 'server-error') and have it render the route named server-error.

Another example would be a /users/:username route where your getInitialAsyncState returns an error and you want to render ´` or similar, even if the route was matched.

@karlmikko
Copy link

@sbrekken How would the async error be handled on the client? One would want to handle these in the same way on the server to keep your code as isomorphic as possible.

@karlmikko
Copy link

I would expect the getInitialAsyncState to return state with an error property. Then in render check for error to show the correct error.

Possibly if you reject the promise ReactRouter could set the state with the error object.

var Foo = React.createClass({
  getInitialAsyncState:function(path, query, setState){
    return new Promise(resolve, reject){
        //if error setState({error:"oh no"});
    }
  },
  render:function(){
    if (this.state.error)
      return <Error.information/>

    if (!this.state.data)
      return <div>Loading...</div>

    return <div>{this.state.data}</div>
  }
});

@ryanflorence
Copy link
Member

@sbrekken

I imagine it working something like this, as posted in #181 (comment) with the ideas from #142 (comment)

// lets say we're in express
app.get('*', function(req, res) {
  Router.renderRoutesToString(routes, req.originalUrl).then(function(result) {
    res.send(result.html);
  }, function(result) {
    if (result.NOT_FOUND)
      res.send(404, result.html);
    else if (result.REDIRECT)
      res.redirect(result.redirectUrl);
    else
      // this would never happen in the client
      res.send(500, result.message);
  });
});

@simenbrekken
Copy link
Contributor Author

@rpflorence Looks good to me, how are you planning on generating result.html?

@mjackson
Copy link
Member

@karlmikko As best I can figure, <Link>s need to know what set of <Routes> they are being rendered inside. Then, instead of listening for changes to the URLStore, all <Link>s rendered underneath the tree of a given <Routes> should be able to use its onActiveStateChange function to update themselves. This does two things for us:

  1. We can get rid of the URLStore entirely because each <Routes> stores its own state internally.
  2. Makes the server-side rendering case a whole lot cleaner.

Edit: I should also say that I think the whole renderRoutesToString implementation should probably wait until this refactoring is done. It will be a whole lot cleaner and easier to follow if we do.

@ryanflorence
Copy link
Member

@sbrekken

how are you planning on generating result.html

It would render just like it can today with * paths. Read the whole comment here

@mjackson 👍

@simenbrekken
Copy link
Contributor Author

@rpflorence Something 🐟'y with your comment link.

@ryanflorence
Copy link
Member

haha, copy/paste fail, link has been updated

@simenbrekken
Copy link
Contributor Author

@rpflorence In your example does a failed promise on the client with result.NOT_FOUND proceed to the closest matching <NotFoundRoute />?

@ryanflorence
Copy link
Member

On the client there would be no failed promise, but yes, it goes to the closes NotFoundRoute so you can have nested UI in your not found views.

<Routes>
  <Route path="/" handler={App}/>
  <Route path="/users/:userId" handler={User}>
    <Route path="/users/:userId/profile" handler={Profile}/>
    <Route path="/users/:userId/*" handler={NotFoundUser}/>
  </Route>
  <Route path="/*" handler={NotFound}/>
</Routes>

^ That works today. /blah would render NotFound where /users/123/blah would render NotFoundUser. Just swap those routes out for <NotFoundRoute handler={NotFoundUser}/>, the path becomes automatically <parent path>/* and now we have a not found handler at any level of your ui.

It also happens to give us a way on the server to know the route is not recognized by the app when rendering on the server, and so we can fail the render promise.

@Swivelgames
Copy link

With love ❤️ , is there any ETA on this perhaps? 😸

Think we jumped the gun a bit and become a little more dependent on this than we were expecting before we realized that this wasn't isomorphic just yet. We're currently trying to use React.transitionTo with React.renderComponentToString() and failing miserably. We're just getting a <noscript/> element instead.

Keep up the great work, guys!

@ryanflorence
Copy link
Member

🎄 ?

no idea ... but definitely before 🎃

@Swivelgames
Copy link

😢 Looks like we'll have to transition to a more traditional router due to the high-visibility and timeline of our project. Good luck with this! I'll definitely be back in the future, personally. Maybe I'll be able to drag the rest of them back with me as well. Thanks @rpflorence 👍

@karlmikko
Copy link

@Swivelgames there is a an Open PR #181 that will enable this, I don't suspect there would be much difference to the API from what is there.

@Swivelgames
Copy link

👍 I'll definitely review it!

@mjackson
Copy link
Member

Let's follow up in #181.

@pedronauck
Copy link

Hey @ryanflorence, the same that ocurred with @th0r throw is ocurring here. That error makes a 500 throw on server routes...

ReferenceError: window is not defined

Do you think that have anything to do in this case?

@burakcan
Copy link

Hello,
I found a way to render on the server with desired data. I don't have time to document it but the idea is basically:

  1. Generate a route list
  2. Spawn a temporary server that serves your app (serve index.html for every page request)
  3. Visit every route in the list with jsdom https://www.npmjs.com/package/jsdom
  4. Throw an event from your app whenever it feels it's ready to be rendered
  5. Listen that event in jsdom and get the html when the event is fired

@lock lock bot locked as resolved and limited conversation to collaborators Jan 24, 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 a pull request may close this issue.