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

when failAction is log or ignored, the error is assigned to request.preResponse but not request.pre #3431

Closed
wbcustc opened this issue Jan 30, 2017 · 12 comments
Assignees
Labels
Milestone

Comments

@wbcustc
Copy link

@wbcustc wbcustc commented Jan 30, 2017

// Route config
{ 
    ...
    pre: [
         { method: 'serializeToS3()', assign: 's3Error', failAction: 'log' },
    ],
}

In the code above, the s3Error is assigned to request.preResponse but not request.pre, I wonder that why in the doc, it just states that the error is assigned but not mention where it is assigned to.

Solution:
Update the hapi doc to explicitly told that the error is assigned to request.preResponse

  • hapi version: 16.0.3
@sirgallifrey
Copy link
Contributor

@sirgallifrey sirgallifrey commented Feb 17, 2017

is this just a documentation thing or does this line needs fixing?

lib/handler.js#L280

should it be something like this?

request.pre[pre.assign] = response instanceof Error ? response : response.source;

I mean, it doesn't seems that the combination of log and assign are leaving the response out of request.pre on purpose

@wbcustc
Copy link
Author

@wbcustc wbcustc commented Feb 17, 2017

I finally moved out from assigning the Error object. I felt like it's not a good practice to do that.
So I chose to use onPreResponse extension to handle all the errors.

But I think for documentation, we definitely want to mention the error assigning behavior.
Also, I think your change is good too.

@sirgallifrey
Copy link
Contributor

@sirgallifrey sirgallifrey commented Feb 17, 2017

IMO using assign should always set the response on request.pre, it is more predictable that way. But of couse, if this is not a bug and this behavior is intended we should put it on the docs definitely!

@psigillito
Copy link

@psigillito psigillito commented Feb 24, 2017

@sirgallifrey So the response.source is always assigned to request.pre and the actual response is always assigned to request.preResponses?

What is the benefit of request.preResponsives in the first place?

Sorry if this is a silly question, I am still learning all of this.

@sirgallifrey
Copy link
Contributor

@sirgallifrey sirgallifrey commented Feb 24, 2017

@Squawk09 You don't have to say sorry for asking a question!

So the response.source is always assigned to request.pre and the actual response is always assigned to request.preResponses?

Yes, we should have the response.source on request.pre and the actual response object on request.preResponse but as we seen in the issue this is not working well with errors.

What is the benefit of request.preResponsives in the first place?

With request.pre you only get the payload, with suits most use cases

//prerequisite 
reply({ something: 'banana' });

// later on handler

console.log(request.pre[nameIAssignedIt]);  // {something:'banana'}

so what about the request.preResponse? let's say you use this awesome plugin to make paginated responses and you use the option to store the pagination metadata on the response header (the actual page, number of pages, pages letf, etc)
If you need the info ( response headers ins this case ) you need to look into request.preResponse

@AdriVanHoudt
Copy link
Contributor

@AdriVanHoudt AdriVanHoudt commented Mar 13, 2017

This comes down to having to check preResponse for errors in most cases no? Doesn't seem handy idd but having to do request.pre[something].source is also a bit ugly :/

@felixheck
Copy link

@felixheck felixheck commented Mar 13, 2017

I like the solution mentioned above because it is backwards compatible..

@sirgallifrey
Copy link
Contributor

@sirgallifrey sirgallifrey commented Mar 13, 2017

@AdriVanHoudt I don't understand why you would have to look into request.pre[something].source could you elaborate a bit more?

@AdriVanHoudt
Copy link
Contributor

@AdriVanHoudt AdriVanHoudt commented Mar 13, 2017

@sirgallifrey misread :3 but if everyone likes @sirgallifrey's proposal a PR will get things moving the quickest I think

@sirgallifrey
Copy link
Contributor

@sirgallifrey sirgallifrey commented Mar 13, 2017

I will happily make a PR with the fix and some regression tests

@AdriVanHoudt
Copy link
Contributor

@AdriVanHoudt AdriVanHoudt commented May 29, 2017

@hueniverse does this mean #3457 is not needed anymore? (also not sure why not just merged that PR?)

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented May 29, 2017

I tend to work from the oldest item in my queue up and sometimes I miss existing PRs...
I haven't got to that PR yet... I'll close it if not needed anymore.

@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 pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants