Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove return value from generateResponse() prepare method #4299

Closed
kanongil opened this issue Oct 9, 2021 · 4 comments
Closed

Remove return value from generateResponse() prepare method #4299

kanongil opened this issue Oct 9, 2021 · 4 comments
Labels
breaking changes Change that can breaking existing code bug Bug or defect

Comments

@kanongil
Copy link
Contributor

kanongil commented Oct 9, 2021

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

hapi/lib/toolkit.js

Lines 104 to 106 in 404d253

if (!response.isBoom && response._state === 'init') {
response = await response._prepare();
}

return this._processors.prepare(this);

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 kanongil added bug Bug or defect breaking changes Change that can breaking existing code labels Oct 9, 2021
@kanongil
Copy link
Contributor Author

kanongil commented 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
Copy link
Member

devinivy commented 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
Copy link
Contributor Author

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.

@devinivy
Copy link
Member

devinivy commented Nov 7, 2022

Resolved with v21 #4386

@devinivy devinivy closed this as completed Nov 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking changes Change that can breaking existing code bug Bug or defect
Projects
None yet
Development

No branches or pull requests

2 participants