Conversation
- removed .use api - removed middie - removed middlewares from request lifecycle - removed middlewares from encapsulation handling - removed kMiddlewares symbol
mcollina
left a comment
There was a problem hiding this comment.
We need to prepare a fastify-middleware implementation (or maybe merge it in middie) and benchmark it. I would expect its overhead to be comparable with our current internal implementation, but it's better to verify.
Note that https://github.com/fastify/fastify-helmet will depend on it, and we cannot easily migrate out of it for some of the compatibility layer.
I think we should add a basic use() method that throw an exception that links to the middie or fastify-express.
- By default .use will throw an error - Decorators can override the default .use - Added new error code
delvedor
left a comment
There was a problem hiding this comment.
We need to prepare a fastify-middleware implementation (or maybe merge it in middie) and benchmark it. I would expect its overhead to be comparable with our current internal implementation, but it's better to verify.
I agree, I'll send a pr to middie and transform it into a plugin.
Note that fastify/fastify-helmet will depend on it, and we cannot easily migrate out of it for some of the compatibility layer.
I think we should add a basic use() method that throw an exception that links to the middie or fastify-express.
Good idea, I've added back the use method, which throws as soon as you use it. Furthermore, the decorator API will not complain if you try to override use.
|
Yes I can update the type defs for this! |
delvedor
left a comment
There was a problem hiding this comment.
I did run some benchmarks, there is no noticeable difference between Fastify v2 and Fastify v3 + middie, which is what we were expecting.
Fastify v2 is slightly slower than Fastify v3 (without middlewares), but performances are not the reason why we are removing core middlewares support.
|
As far as I can tell the only TS modification needed was in the All of the current hook handlers types currently return an instance of FastifyMiddleware. This seems like an issue with the type system. I think a lot of this stuff was mainly rolled over from the original types so we can take this opportunity to clean it up. What should be returned from every route level hook? Whatever the answer is to that question will help me change the type system to better reflect the intended behavior here. |
|
@Ethan-Arrowood I think we should get rid of the Furthermore, I'm not sure what it's the meaning of the Lines 21 to 25 in 047442b Every hook gets the Fastify request/reply objects, and you can access the Node.js's request object via |
Sounds good. I'll start a branch to fix this up.
These are all generic parameters the type system uses to enforce the same Server/Request/Reply types throughout the type defined server. It makes the type system slightly more confusing, but the user experience is way improved because the user only needs to specify what it is way at the beginning. I'll have something started as soon as possible |
|
Code is ok. I would wait for a solution to avoid the after call. |
|
@delvedor do Route level hooks return anything? I know the application hooks return |
|
After reading the |
|
@delvedor wanted to let you know types have finished being updated. I linked a draft PR if we want to merge separately. Otherwise happy to push that work to your branch if you'd like. |
|
@Ethan-Arrowood we can directly merge in |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
@Eomm could you fix the conflicts? :) |
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Hello folks! As you know, we have decided to remove middlewares support from Fastify core and move it to an external plugin, which will be middie and fastify-express.
If you want to know more about this decision, take a look at #1503.
In this pr, we are removing any line of code that handled our middlewares, the request lifecycle has been updated, same for test and documentation.
@Ethan-Arrowood would you like to work on the type definitions?
All the tests that I have removed from Fastify core will be added to fastify-express and middie, so we can be sure that the behavior of the API will be the same.
Closes: #1503
Checklist
npm run testandnpm run benchmark