hapi icon indicating copy to clipboard operation
hapi copied to clipboard

Remove return value from generateResponse() prepare method

Open kanongil opened this issue 4 years ago • 3 comments

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).

https://github.com/hapijs/hapi/blob/404d2534e73350eb25188bd06fa638f74f8f54e4/lib/toolkit.js#L104-L106 https://github.com/hapijs/hapi/blob/404d2534e73350eb25188bd06fa638f74f8f54e4/lib/response.js#L510

Returning anything other than the existing response is however fundamentally broken since:

  1. Returned value is not validated for correctness.
  2. It causes the close() option to never be called.
  3. 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 the prepare() 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.

kanongil avatar Oct 09 '21 15:10 kanongil

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.

kanongil avatar Oct 09 '21 15:10 kanongil

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.

devinivy avatar Oct 09 '21 16:10 devinivy

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.

kanongil avatar Oct 10 '21 00:10 kanongil

Resolved with v21 https://github.com/hapijs/hapi/issues/4386

devinivy avatar Nov 07 '22 14:11 devinivy