feat(express, koa): make transports similar#2486
Conversation
afb67f2 to
ba80649
Compare
|
|
||
| const protoService = wrapService(location, service, options); | ||
| const serviceOptions = getServiceOptions(service, options); | ||
| const serviceOptions = getServiceOptions(protoService); |
There was a problem hiding this comment.
Just a note for myself while I review: Since wrapService already calls getServiceOptions with the options, we don't need it here.
There was a problem hiding this comment.
Alright, I wanted to take the time do a thorough review since this might have some backwards compatibility implications. Apologies it took so long, this is pretty awesome. My first concern was that the setup order that is required now would break existing generated applications but it looks like middleware is already set up in the correct order so this would just be something that needs to be mentioned in the migration guide.
My next question would be if this change would break any other expectations in existing applications. If routing is not done by Express, would things like app.use('*') and app.use('/existingservice', (req, res, next) => {}) still work?
Other than pointing out the order, is there anything else that needs to be updated in the documentation?
Oh also, I invited you to the organization, which should make it easier to collaborate on PRs 😄
|
|
||
| if (authentication) { | ||
| debug('Parsed authentication from HTTP header', authentication); | ||
| req.feathers = { ...req.feathers, authentication }; |
Yes, this is a tested feature and if something passed to But the order has been changed. Currently if you want to run this middleware before "existingservice" you need to put it before the service is registered, now instead you need to do it before
I listed breaking changes for express that I know of.
I of course accept the badge of honor, but how does membership helps with collaboration? |
57075e0 to
af6f70c
Compare
|
So after the discussion in Slack, I am going to merge this and see what happens 😄 we can always revert the routing or create a compatibiility wrapper but @vonagam was making a good point in the value of having the transports similar. Plus we can sell it as a performance improvement since our router is much faster than Express internal routing. Thanks again for doing this, some great improvements in there! |
There are a few differences between express and koa servers. This PR tries to bring them closer.
There are some breaking changes for express (and koa, but since it appeared only in this version those do not matter much). Main ones:
.configure(rest())matters and it should go afterjson()middleware (or others of such sorts).middlewareinServiceOptionsrenamed toexpress(to differentiate from middleware for other transports).authenticatemiddleware merges auth result intorequest.feathers(like it should) not request itself.List of changes (in randomish order, excluding some small things):
settings.strategiesinparseAuthentication. (should be here, in koa it is used.)authenticatemiddleware, renameauthenticationtoparseAuthentication.authenticatemiddleware now is a wrapper aroundauthenticatehook, so behaves the same (before there were some inconsistencies).koa.beforeandkoa.after.express.composedandkoa.composed(can use unique symbol instead).getServiceOptions(before this line used an original unwrapped service and so was creating a copy).transport-commons(like koa and sockets).lookupresult onreq/ctx(available for service middleware).req.feathersdoes not containheadersby default, onlyprovider.rest()is a configure function now, not koa middleware, so should be used with.configure(), not.use(). A configure function is a more flexible approach (allows to add a mixin for example) and does not reveal implementation.rest()addsparseAuthenticationmiddleware just like express does right now.rest()accepts formatter middleware that is a last step in service call processing, by default is an empty function, but it means that it does not callnext(), so no falling through like right now (and consistent with express).Aim is to remove differences in code as much as possible between those two transports, simplifying end-user migrations and easing porting features.