React-router: Feature request: Promise support in getComponent

[LinusU]

I think it would be great to support promises in the getComponent function. This would be very useful with the new loader api and especially with webpack 2.0 which will support ES6 modules natively.

That way we could simplify code-splitting when using Webpack.

const route = {
  path: 'company/:companyId',
  getComponent: () => System.import('views/company')
}

Instead of how it is now:

const route = {
  path: 'company/:companyId',
  getComponent (nextState, cb) {
    System.import('views/company').then(
      res => setTimeout(cb, 0, res),
      err => setTimeout(() => throw err, 0)
    )
  }
}

(the setTimeout is required to schedule the callback and the throw in another tick since otherwise any errors will be catched and used to reject a promise that will be thrown away, thus silencing the error)

This feature could be implemented with backwards compatibility by checking if the return value is a Promise and using that to opt into the new behaviour.

[taion]

It's a good idea. Sadly we can't drop the current API (even for v3) because most people are still going to be on webpack 1.

[taion]

cc @mhart

Unfortunately this means we really need to strip out the stack size enhancements from https://github.com/reactjs/react-router/issues/2922 – the maintenance overhead of keeping around the special handling there plus supporting both a cb and a promise API would be too much.

I've personally never needed to set a breakpoint in that code in either using or hacking on React Router, so I think in practice it's not that big a deal, especially given that we've observed no performance penalty from the stack depth.

[mhart]

Ah well. The performance penalty isn't too much of an issue, agreed, but the stack depth will definitely get in the way of anyone doing performance profiling on the server (even if they're not profiling react router itself, they'll need to wade through the whole chain of calls before they get to their request handling code).

I'm not religiously in favour of keeping it though, so y'all gotta do what you gotta do 👍

[wtgtybhertgeghgtwtg]

Sadly we can't drop the current API (even for v3)

If not dropped, would the callback API be deprecated? Or would that be over-eager?

[taion]

I don't think we can deprecate or drop the callback-based API. webpack 2 isn't even released yet, and the callback-based API is more natural with require.ensure in webpack 1.

[rochdev]

I don't see any reasons not to keep support for both approaches long-term.

[taion]

It's never ideal to have two ways to do the same thing.

[rochdev]

While I generally agree, this is a common pattern for asynchronous programming in node. It's used by RethinkDB, Mocha and I remember seeing it in quite a few other libraries. I wouldn't cry if callbacks were dropped eventually though :P

[taion]

Either way – it's not going to matter for a while 😃

[timdorr]

FWIW, @mjackson hates Promises 😜

[taion]

Nothing's perfect, but the System.import use case is pretty compelling.

The idea would be that, like with graphql-js, we'll support returning either a (non-thenable) value, a thenable, or returning undefined and hitting the callback.

[taion]

Folded into https://github.com/reactjs/react-router/issues/3611.

[KeKs0r]

I am not sure if that would be in scope for this, but I think it would be interesting to allow getChildRoutes to return an array of promises. I ran into several use cases where that would be neat. Especially for code splitting with nested asynchronous routes.

[taion]

Just use Promise.all...

[timdorr]

Added in #3719!

[Gerhut]

Why not implement it directly in component?

const route = {
  path: 'company/:companyId',
  component: System.import('views/company')
}

[taion]

That will load the component eagerly.