Repository navigation
Conversation
Closes fastify#6284 addHttpMethod could silently reconfigure built-in HTTP methods (e.g. GET, POST) without any indication to the developer. The name 'add' implies adding new methods, not overriding existing ones. This commit introduces a new option (default: false). When a method that already exists in is passed without , a deprecation warning (FSTWRN005) is emitted. The actual reconfiguration still proceeds so this is fully non-breaking in v5. In Fastify v6, calling addHttpMethod on an existing method without { overrideExisting: true } will throw an error. Changes: - lib/warnings.js: add FSTWRN005 warning - fastify.js: add overrideExisting option and emit FSTWRN005 when needed - types/instance.d.ts: update addHttpMethod signature - test/http-methods/custom-http-methods.test.js: add 3 new tests - test/types/instance.test-d.ts: add type tests for the new option
|
We are late to the party. I think the PR is good, but could you resolve the conflicts? |
jean-michelet
left a comment
jean-michelet
left a comment
There was a problem hiding this comment.
I would still prefer add a new setHttpMethod and remove addHttpMethod in future major.
But if other contributors agree with this, it's already better then the current misleading implementation.
| const FSTWRN005 = createWarning({ | ||
| name: 'FastifyWarning', | ||
| code: 'FSTWRN005', | ||
| message: 'addHttpMethod was called for method "%s" which already exists. Pass { overrideExisting: true } to suppress this warning. In Fastify v6, calling addHttpMethod on an existing method without this option will throw an error.', |
There was a problem hiding this comment.
Do we agree about this behavior in next major @fastify/core?
|
Hi @jean-michelet @Eomm, Thanks for the feedback! I've resolved the merge conflicts with Let me know if there's anything else needed to get this merged! |
jean-michelet
left a comment
jean-michelet
left a comment
There was a problem hiding this comment.
Ok, but let's remove useless coments plz.
| } | ||
| process.on('warning', onWarning) | ||
|
|
||
| // POST is a built-in bodywith method — with the flag, no warning should fire |
There was a problem hiding this comment.
| // POST is a built-in bodywith method — with the flag, no warning should fire |
| t.plan(2) | ||
| const fastify = Fastify() | ||
|
|
||
| // GET is bodyless by default; override it to accept a body |
There was a problem hiding this comment.
| // GET is bodyless by default; override it to accept a body |
| t.plan(2) | ||
| const fastify = Fastify() | ||
|
|
||
| // Attach listener BEFORE calling addHttpMethod — process.emitWarning is async (next tick) |
There was a problem hiding this comment.
| // Attach listener BEFORE calling addHttpMethod — process.emitWarning is async (next tick) |
|
@Vansh0204 can you plz fix the conflicts and remove comments plz? |
|
Sorry, I let you 4 days to fix but we now must ship that. |
|
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. |
Summary
Closes #6284
addHttpMethodcould silently reconfigure built-in HTTP methods (e.g.GET,POST) without any indication to the developer. The function name implies adding new methods, not overriding existing ones.Changes
This PR introduces a new
overrideExistingoption (default:false) toaddHttpMethod.kSupportedHTTPMethodsis passed withoutoverrideExisting: true, a deprecation warning (FSTWRN005) is emitted.addHttpMethodon an existing method without{ overrideExisting: true }will throw an error.Usage
Files Changed
lib/warnings.jsFSTWRN005warningfastify.jsoverrideExistingoption toaddHttpMethodtypes/instance.d.tstest/http-methods/custom-http-methods.test.jstest/types/instance.test-d.ts