Repository navigation
Remove return value from generateResponse() prepare method #4299
Description
Support plan
- is this issue currently blocking your project? (yes/no): no
- is this issue affecting a production system? (yes/no): probably not
Context
- node version: any
- module version: 20.2.1
- environment (e.g. node, browser, native): node
- used with (e.g. hapi application, another framework, standalone, ...):
- any other relevant information:
What are you trying to achieve or the steps to reproduce?
The request.generateResponse() takes a prepare(response) option that must return a response (existing or new).
Lines 104 to 106 in 404d253
Line 510 in 404d253
Returning anything other than the existing response is however fundamentally broken since:
- Returned value is not validated for correctness.
- It causes the
close()option to never be called. - It is not feasible to actually create, since there is no Toolkit to create it from. Only
request.generateResponse()is available, and if used can break since theprepare()option is never called for it.
Given the above, I expect the feature has never been used and I suggest it is simply removed. Especially since there is no test code that covers this, or any pertinent use cases. Any return value should just be ignored. The feature is likely a vestige from when returned errors would be handled.
FYI the generateResponse() prepare option is currently only used in inert and vision.
Activity
kanongil commentedon Oct 9, 2021
While this is definitely a breaking change (the docs will have to change), I feel that it would be fair to remove such a broken feature on the current release.
devinivy commentedon Oct 9, 2021
I fully agree that this is broken, but I do have some concerns about removing it in v20. If someone is using the functionality, updating this and changing their responses could have worse effects for them than the bugs mentioned above. And if nobody is using it, then this issue doesn't affect anyone. I would be all for updating the docs now to discourage the behavior, then updating the code in v21. I also would be open to handling the case of no return value in v20 by assuming the response hasn't been changed.
kanongil commentedon Oct 10, 2021
I guess you are right. My main concern is that someone might add new code that tries to depend on this. But that is probably quite unlikely.
Soft deprecate returning a new object. See hapijs#4299 for details
Soft deprecate returning a new object. See #4299 for details (#4302)
devinivy commentedon Nov 7, 2022
Resolved with v21 #4386