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

Error when reply() is called with a third argument (non-auth) #3303

Closed
hueniverse opened this issue Aug 22, 2016 · 5 comments
Closed

Error when reply() is called with a third argument (non-auth) #3303

hueniverse opened this issue Aug 22, 2016 · 5 comments
Assignees
Milestone

Comments

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Aug 22, 2016

Currently, the third argument is only used by the authentication strategy logic and is ignored elsewhere. This is to ensure the interfaces are used correctly and prevent bugs. Note that if you pass the result of a cached method directly to reply, you will need to drop the third ttl argument.

@hueniverse hueniverse added this to the 15.0.0 milestone Aug 22, 2016
@hueniverse hueniverse self-assigned this Aug 22, 2016
@hueniverse
Copy link
Contributor Author

@hueniverse hueniverse commented Aug 22, 2016

See 2091313#diff-4714f65d47d46b2330b6dce750b931d5R2439 for how to change your code to handle the third argument.

Loading

@hueniverse
Copy link
Contributor Author

@hueniverse hueniverse commented Aug 22, 2016

I'm looking for feedback on this issue. It's better to throw but this might be a painful change for some if you chain methods into reply() directly and this would require a messy refactor (in which case the bug safety gain is minimal).

Loading

@hueniverse hueniverse reopened this Aug 22, 2016
@AdriVanHoudt
Copy link
Contributor

@AdriVanHoudt AdriVanHoudt commented Aug 22, 2016

I am +1 on this, the example is a really good way to deal with it

Loading

@devinivy
Copy link
Member

@devinivy devinivy commented Aug 23, 2016

Fine with me. I'll admit though, it is too bad that the reply interface is setup to be able to be used as a standard callback but that server methods call-back with a third argument– it would be nice for them to be compatible.

Could server methods actually check if the callback is a reply interface, then call it with two arguments if so? Seems a little quirky, but we do a similar thing with boom errors (err.isBoom) all the time. It would be a minor-level change, so we could make a decision on that at a later date without any trouble.

Loading

@hueniverse hueniverse reopened this Aug 23, 2016
@hueniverse
Copy link
Contributor Author

@hueniverse hueniverse commented Aug 23, 2016

Commit above was a typo.

Loading

@hueniverse hueniverse closed this Aug 23, 2016
@lock lock bot locked as resolved and limited conversation to collaborators Jan 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants