-
-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
Feature request: Promise support in getComponent #3606
Comments
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. |
cc @mhart Unfortunately this means we really need to strip out the stack size enhancements from #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. |
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 👍 |
If not dropped, would the callback API be deprecated? Or would that be over-eager? |
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 |
I don't see any reasons not to keep support for both approaches long-term. |
It's never ideal to have two ways to do the same thing. |
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 |
Either way – it's not going to matter for a while 😃 |
FWIW, @mjackson hates Promises 😜 |
Nothing's perfect, but the The idea would be that, like with graphql-js, we'll support returning either a (non-thenable) value, a thenable, or returning |
Folded into #3611. |
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. |
Just use |
Added in #3719! |
Why not implement it directly in component? const route = {
path: 'company/:companyId',
component: System.import('views/company')
} |
That will load the component eagerly. |
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.
Instead of how it is now:
(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.
The text was updated successfully, but these errors were encountered: