-
Notifications
You must be signed in to change notification settings - Fork 3k
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/ajax response headers #6057
Conversation
bf93b90
to
bba710f
Compare
Ugh... |
No biggy... it's probably more efficient as a reduce :) |
cd4e448
to
e5dcb0d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but maybe use a getter?
this.responseHeaders = allHeaders | ||
? // Split the header text into lines | ||
allHeaders.split('\n').reduce((headers: Record<string, string>, line) => { | ||
// Split the lines on the first ": " as | ||
// "key: value". Note that the value could | ||
// technically have a ": " in it. | ||
const index = line.indexOf(': '); | ||
headers[line.slice(0, index)] = line.slice(index + 2); | ||
return headers; | ||
}, {}) | ||
: {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could implement responseHeaders
as a getter so that the headers don't have to be parsed for every response?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did that initially, but then I had a add a private property, tests got messy, I got lazy because it was 2am, etc. Happy to take another pass at that in a follow up PR, though.
Adds a feature that allows the user to examine response headers more easily (based loosely off of prior art from contemporary libraries):