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

feat: pass response status & request details to beforeRedirect #198

Merged
merged 3 commits into from
May 3, 2022

Conversation

davecardwell
Copy link
Contributor

@davecardwell davecardwell commented Apr 26, 2022

I have a couple of use cases† that require the beforeRedirect hook to be aware of the requested URL that is resulting in the redirect. I also noted that in #191 @SergeBabich wanted to add the redirect status code, but hadn’t provided tests.

This PR adds both these to the response details object passed to beforeRedirect.

Update (1aa5e3c) - I also include the request method.
Update (9771127) - Moved URL and method to a third “request details” param
Update (f63ebbc) - Added the request headers to the third param

† I need to store cookies returned across a chain of redirects, as well as to record some forensics about each individual request/response in the chain.

This commit adds the originally requested URL and the redirect
response’s HTTP status code to the response details passed to
`beforeRedirect`.
Copy link
Collaborator

@RubenVerborgh RubenVerborgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, good idea but let's attach a third request parameter to the beforeRedirect callback.

README.md Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@davecardwell davecardwell changed the title feat: pass response status & request URL to beforeRedirect feat: pass response status & request details to beforeRedirect Apr 27, 2022
@SerhiiBabich
Copy link

Yes, I did PR, but still have no time to add tests

@davecardwell
Copy link
Contributor Author

@SergeBabich No problem. This PR should add the extra field you wanted once merged!

@SerhiiBabich
Copy link

@SergeBabich No problem. This PR should add the extra field you wanted once merged!

You rock!

@RubenVerborgh
Copy link
Collaborator

Do we really need the request headers? getRawHeaderNames is a fairly recent addition.
I'll roll back for now; can be added later if needed.

@RubenVerborgh RubenVerborgh merged commit 24dcb20 into follow-redirects:main May 3, 2022
@davecardwell
Copy link
Contributor Author

Do we really need the request headers? getRawHeaderNames is a fairly recent addition. I'll roll back for now; can be added later if needed.

@RubenVerborgh I would find the request headers useful for our purposes (recording requests with axios-har-tracker). Would you be happy with just using getHeaderNames(), added in Node.js v7.7+? I can switch to that if that would be acceptable.

@RubenVerborgh
Copy link
Collaborator

Ah shoot I just merged at the exact same time as your comment!

@RubenVerborgh
Copy link
Collaborator

@davecardwell Thanks for your work, I'll take up the headers stuff.

@davecardwell
Copy link
Contributor Author

@RubenVerborgh Thank you very much for helping get this across the line! It’s going to be a big help to us!

@davecardwell davecardwell deleted the feat/request-url branch May 3, 2022 21:03
@RubenVerborgh
Copy link
Collaborator

@davecardwell Even getHeaderNames is v7.7.0; we unfortunately need to support all the way back to v4. I'll find another way.

@RubenVerborgh
Copy link
Collaborator

Done in 96a3947.

@davecardwell
Copy link
Contributor Author

@RubenVerborgh Awesome, thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants