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

allow decoration of reply with non functions #3519

Merged
merged 1 commit into from Jul 17, 2017
Merged

Conversation

@gmauricio
Copy link
Contributor

@gmauricio gmauricio commented Jun 8, 2017

After 16.2.0, it's not possible to decorate the reply object with something different to a function. This is due to the work originated on #3448 where all reply methods are being bound to the request domain.
After that @hueniverse made a change to make sure that decorator methods are also bound to the domain, but it expects that any decoration added to the reply is also a function.
We were able to fix our issue by checking first that the decoration is in fact a function, and only bound it if that's the case.
I'm not sure if this is really a fix and if it's expected to be able to decorate the reply with something different to a function. I see that you always name the decoration as "method", but on the docs you mention the method argument for the decorate function can be

the extension function or other value

which makes me think that adding anything that's not a function is expected.
If the accepted behavior is to only allow functions to decorate the reply, then I'd be happy to contribute by adding an assertion on decorate to expect only functions and updating the docs to make that clear.

lib/reply.js Outdated
@@ -105,7 +105,7 @@ internals.Reply.prototype.interface = function (request, realm, options, next) {
for (let i = 0; i < methods.length; ++i) {
const method = methods[i];
const decoration = this._decorations[method];
reply[method] = (domain ? domain.bind(decoration) : decoration);
reply[method] = domain && typeof decoration === 'function' ? domain.bind(decoration) : decoration;
Copy link
Contributor

@AdriVanHoudt AdriVanHoudt Jun 8, 2017

I would add () around domain && typeof decoration === 'function' to make it clearer
LGTM otherwise

@hueniverse hueniverse self-assigned this Jul 17, 2017
@hueniverse hueniverse added the bug label Jul 17, 2017
@hueniverse hueniverse added this to the 16.4.4 milestone Jul 17, 2017
@hueniverse hueniverse merged commit 970f3b4 into hapijs:master Jul 17, 2017
2 checks passed
@lock
Copy link

@lock lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants