Add Promise support to async route functions #3719
Conversation
| @@ -0,0 +1,3 @@ | |||
| export function isPromise(obj) { | |||
| return obj && typeof obj.then === 'function' | |||
| } | |||
There was a problem hiding this comment.
Dunno if this is worth throwing in a module. Maybe move it to AsyncUtils at least?
There was a problem hiding this comment.
This is okay for now I think.
| // Try module.default first in case of System.import and Babel 6 | ||
| .then(component => component.default || component) | ||
| .then(component => callback(null, component)) | ||
| .catch(callback) |
There was a problem hiding this comment.
I think this should be
.then(
component => callback(null, component),
callback
)We don't want to catch exceptions thrown in running the callback.
|
Super neat! |
| if (isPromise(componentReturn)) | ||
| componentReturn | ||
| // Try module.default first in case of System.import and Babel 6 | ||
| .then(component => component.default || component) |
There was a problem hiding this comment.
I don't think it's Babel 6 – I think that's just what System.import does, no? What happens if there are non-default exports.
I wonder if we should handle this though. Presumably users can do
function asyncComponent(id) {
return System.import(id).then(component => component.default);
}There was a problem hiding this comment.
I haven't followed the spec discussions closely enough to know what the standard implementation will do. But since Babel is the only way to consume exports at the moment, it's realistically how things are going to be for the next few years (at least). So this is more of a practical enhancement to be a little more friendly to users.
There was a problem hiding this comment.
I feel like that should live in user space though. We don't have anything like this in the callback-based path. I do agree that this is friendlier, but I think philosophically it's not right here.
Also, webpack 2 can handle these things natively now... users on webpack 2 actually are using that native ES module support now that we have the module entry point.
There was a problem hiding this comment.
(but the API for them looks the same)
There was a problem hiding this comment.
The other thing is that the spec doesn't really define how System.import works. Probably you'd using it with webpack 2, which takes a guess, but isn't actually going through Babel. It might make a lot of sense to change this down the road, but I think we should at least see how it plays initially in the simpler form, and reserve room to change up how it works as the spec for System.import comes into existence.
There was a problem hiding this comment.
OK, not a huge deal to me, so I dropped them. But you get the blame if users complain :P
|
Browser Stack wigged out again. Build is passing now. |
|
So are we yay or nay? |
|
Looks great! |
|
Thanks! 🎉 I'll cut another alpha once we figure out if we want #3717. |
|
Since 1ebbf58 the initial example isn't valid any more, you probably need to do this: <Route path='/' getComponent={() => System.import('./app').then(poo => poo.default)} /> |
First pass at #3606. Not 100% happy with it right now. It would be easier to architect if we had guaranteed Promise availability, but alas...
I've got some practical things in there for Babel's
defaultexports. And we can't just accept a Promise instead of a function, sinceSystem.importwon't code split in that case. That's probably the most ugly part of it.But this works: