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

Expand Headers types #4144

Merged
merged 12 commits into from Nov 16, 2021
Merged

Expand Headers types #4144

merged 12 commits into from Nov 16, 2021

Conversation

bfaulk96
Copy link
Contributor

@bfaulk96 bfaulk96 commented Oct 5, 2021

Update headers type for a request to allow boolean and number types in addition to strings.
Export RequestHeaders and ResponseHeaders type aliases

@bfaulk96 bfaulk96 mentioned this pull request Oct 6, 2021
@caugner
Copy link
Contributor

caugner commented Oct 6, 2021

Afaik HTTP headers are always transferred as strings.

Could you elaborate on which headers accept booleans and numbers and why these cannot be provided as a string?

@bfaulk96
Copy link
Contributor Author

bfaulk96 commented Oct 6, 2021

This update to require Record<string, string> broke backwards compatibility – booleans and numbers all toString() cleanly, so it makes this library a lot more user-friendly to accept other primitive types

i.e.

const headerA = 5; // simplified for example, imagine all of these headers came from something 
const headerB = false;
const headerC = 'someStr';
await axios.get(someUrl, { headers: { headerA, headerB, headerC } });

is a lot simpler from a user's perspective than

const headerA = 5; // simplified for example, imagine all of these headers came from something 
const headerB = false;
const headerC = 'someStr';
await axios.get(someUrl, { 
  headers: {
    headerA: headerA.toString(), 
    headerB: headerB.toString(), 
    headerC 
  } 
});

@remcohaszing
Copy link
Contributor

I thought about this as well, but I concluded that since HTTP headers are strings by definition and the types should reflect this. Also none of the official HTTP headers support the values true, false, or undefined, Internally axios doesn’t convert the header types in any way It just passes them directly into request.setRequestHeader(key: string, val: string).

There are some headers that support numeric strings, for example Access-Control-Max-Age. Although I don’t think axios should support numbers, I can see why this is convenient. If this is supported, I think it should be explicit (documented and tested).

@bfaulk96
Copy link
Contributor Author

bfaulk96 commented Oct 6, 2021

@remcohaszing I can agree with that overall, makes sense. I guess my pain point is that this can be seen as a regression for any applications that were already using it in that way – If an app previously passed "Access-Control-Max-Age": 5 into Axios when the type was any, the 0.22.0 update now shows a TS error instead of converting it to a string for the user.

@caugner
Copy link
Contributor

caugner commented Oct 8, 2021

I thought about this and think it would make sense to also allow boolean and number values, but not undefined. My guess is that booleans will be converted to "0" or "1" and numbers accordingly, even if not by axios itself. But true, that would need to be tested.

@bfaulk96 Could you verify this using the network tab of your favorite browser's Dev Tools?

PS: You might want to rebase your branch on latest master, or merge latest master, as #4136 introduced two interface AxiosRequestHeaders and AxiosResponseHeaders, and your changes should only affect AxiosRequestHeaders.

@remcohaszing
Copy link
Contributor

I thought about this and think it would make sense to also allow boolean and number values, but not undefined. My guess is that booleans will be converted to "0" or "1" and numbers accordingly, even if not by axios itself. But true, that would need to be tested.

Values are simply cast to string. true becomes 'true', false becomes 'false', objects become '[Object object]', etc.

I think the axios maintainers should determine what values they want to support and the types should reflect this.

@bfaulk96
Copy link
Contributor Author

bfaulk96 commented Oct 8, 2021

I thought about this and think it would make sense to also allow boolean and number values, but not undefined. My guess is that booleans will be converted to "0" or "1" and numbers accordingly, even if not by axios itself. But true, that would need to be tested.

Agreed. I got a little carried away with undefined, as that doesn't toString() nicely.

To answer the other question, yes:
5 -> "5"
true -> "true"

Those seem fairly harmless to allow – but I surely understand if not.

On the note of perhaps a a feature request – would be cool to allow Date but would require a transformer, as HTTP date headers are different format than date's toString. Would be nice though :D

@bfaulk96
Copy link
Contributor Author

bfaulk96 commented Oct 8, 2021

Also, updated this branch to reflect our conversation – used the latest types already existing in master, and removed undefined as an allowed type.

Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

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

LGTM

@bfaulk96
Copy link
Contributor Author

Merged in latest changes 👍

Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

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

LGTM

@bfaulk96
Copy link
Contributor Author

Any update on this one? Do I need to do something/reach out to somebody? Or just be patient? 😄

@remcohaszing
Copy link
Contributor

@jasonsaayman what do you think?

Currently users have to explicitly pass strings as headers:

const maxAge = 1337;
const customHeader = true;

axios.get('/api', {
  headers: {
    'Access-Control-Max-Age': String(maxAge),
    'X-Custom-Header': String(customHeader)
  }
})

Basically these types say the following is also acceptable:

axios.get('/api', {
  headers: {
    'Access-Control-Max-Age': maxAge,
    'X-Custom-Header': customHeader
  }
})

@bfaulk96
Copy link
Contributor Author

@jasonsaayman any thoughts on this?

@jasonsaayman
Copy link
Member

@bfaulk96 can you please have a look at the failing tests?

@bfaulk96
Copy link
Contributor Author

Done – sorry about that

@bfaulk96
Copy link
Contributor Author

bfaulk96 commented Nov 3, 2021

@jasonsaayman Any chance of getting this merged?

@bfaulk96
Copy link
Contributor Author

bump

@jasonsaayman
Copy link
Member

Yip will merge today

@jasonsaayman jasonsaayman merged commit 6b4fd93 into axios:master Nov 16, 2021
@bfaulk96 bfaulk96 deleted the patch-1 branch November 16, 2021 15:07
@estarossa0
Copy link
Contributor

shouldn't this changes be added to response headers as well ?

@bfaulk96
Copy link
Contributor Author

bfaulk96 commented Dec 10, 2021

shouldn't this changes be added to response headers as well ?

No, since HTTP Headers are always sent as strings, Response headers will be strings that you will need to parse if you want different types from them
For example, if I send "some-number": 5, the endpoint receiving my headers will receive "some-number": "5"

@bfaulk96
Copy link
Contributor Author

@jasonsaayman Any estimate on when the next Axios release might be?

@estarossa0
Copy link
Contributor

@jasonsaayman Any estimate on when the next Axios release might be?

#4379 (comment)

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.

None yet

5 participants