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

Add response headers to all responses #932

Closed
wants to merge 10 commits into from

Conversation

denysoblohin-okta
Copy link
Contributor

@denysoblohin-okta denysoblohin-okta commented Sep 8, 2021

Expose request headers in new responseHeaders property. Value is object filled with response headers key-value pairs.
This can be useful to get X-Rate-Limit-* headers as requested in #230

Another solution could be exposing the whole Response object instead (eg. in property rawResponse, then headers could be accessed with Headers API).
But that approach leads to problems with unit testing and caching responses (complications with serialization and mocking Fetch API objects).

Internal ref: OKTA-412511
Resolves #230

Important
Most headers are not accessible in browsers due to CORS. (In Node environment it works fine)
Access-Control-Expose-Headers should be set from server-side to allow web clients to use X-Rate-Limit-* etc. headers

@codecov-commenter
Copy link

codecov-commenter commented Sep 8, 2021

Codecov Report

Merging #932 (fda719d) into master (c2d1f56) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #932      +/-   ##
==========================================
+ Coverage   91.97%   91.98%   +0.01%     
==========================================
  Files         120      120              
  Lines        3400     3405       +5     
  Branches      699      700       +1     
==========================================
+ Hits         3127     3132       +5     
  Misses        273      273              
Impacted Files Coverage Δ
lib/fetch/fetchRequest.ts 100.00% <100.00%> (ø)
lib/http/request.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c2d1f56...fda719d. Read the comment docs.

const result: HttpResponse = {
responseText: isObject ? JSON.stringify(data) : data as string,
status: status
status: status,
responseHeaders
Copy link
Contributor

Choose a reason for hiding this comment

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

how about name it as headers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed

@denysoblohin-okta denysoblohin-okta force-pushed the od-OKTA-412511-RAWRESPONSE branch from dca6b15 to f805edf Compare September 10, 2021 16:35
@denysoblohin-okta denysoblohin-okta marked this pull request as ready for review September 10, 2021 16:35
@denysoblohin-okta denysoblohin-okta changed the title Add responseHeaders to all responses Add response headers to all responses Sep 10, 2021
Copy link
Contributor

@shuowu shuowu left a comment

Choose a reason for hiding this comment

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

Please also add changelog (new feature)

@denysoblohin-okta denysoblohin-okta force-pushed the od-OKTA-412511-RAWRESPONSE branch from f805edf to 4bedbf2 Compare September 13, 2021 11:06
@denysoblohin-okta denysoblohin-okta force-pushed the od-OKTA-412511-RAWRESPONSE branch from 4bedbf2 to a0e5b10 Compare September 13, 2021 13:22
eng-prod-CI-bot-okta pushed a commit that referenced this pull request Sep 13, 2021
OKTA-412511
<<<Jenkins Check-In of Tested SHA: a0e5b10 for [email protected]>>>
Artifact: okta-auth-js
Files changed count: 15
PR Link: "#932"
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.

Adding ability to get rate limit values .
4 participants