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

fix(types): allow to specify partial default headers for instance cre… #4185

Merged
merged 2 commits into from May 9, 2022

Conversation

lukashroch
Copy link
Contributor

@lukashroch lukashroch commented Oct 13, 2021

Although PR #4140 mentioned it solved the #4117 PR, it didn't include changes to axios instance creation.

E.g. you can't still do the following as it still expects to provide full provide full HeadersDefaults interface rather a partial one.

axios.create({ headers: { common: { 'some-header': 'some-value' } } }); <-- thrown type error

It should be possible to just provide partial one (Partial<HeadersDefaults>) as it's merged with defaults anyway.

@remcohaszing
Copy link
Contributor

remcohaszing commented Oct 13, 2021

I think the RequestHeaders type needs to be changed to:

type RequestHeaders = Record<string, string> | {
    common?: Record<string, string>;
    delete?: Record<string, string>;
    get?: Record<string, string>;
    head?: Record<string, string>;
    link?: Record<string, string>;
    options?: Record<string, string>;
    patch?: Record<string, string>;
    post?: Record<string, string>;
    purge?: Record<string, string>;
    put?: Record<string, string>;
    unlink?: Record<string, string>;
}

Then AxiosDefaults and CreateAxiosDefaults both need to be replaced with RequestHeaders.

playground link

I just tried this and it appears to correctly reflect the runtime behaviour regardless if the headers are set as axios.defaults.headers, passed to axios.create(), or passed to axios.request(). Also passing a string as headers.common, headers.get, etc appears to be problematic, which is also reflected by these types.

@lukashroch
Copy link
Contributor Author

lukashroch commented Oct 14, 2021

I'm not sure AxiosDefaults that should be all optional... see https://github.com/axios/axios/blob/master/lib/defaults.js#L126 and below. Couple of methods have defaults (either empty object or some default in-built headers), which are merged with whatever you pass in later during axios instantiation. So these method headers are always there.

@remcohaszing
Copy link
Contributor

remcohaszing commented Oct 14, 2021

I see what you’re saying. This is the exact reason I made the default headers optional for some methods, and required for others initially.

Axios still works fine after deleting those though. I also feel like the intent should be that axios.defaults is just the default value of AxiosRequestConfig for the default axios instance.

I.e. this works fine:

const axios = require('axios');

delete axios.defaults.headers.common;
delete axios.defaults.headers.get;

axios.get('https://example.com').then((response) => {
  console.log(response.data);
});

@lukashroch
Copy link
Contributor Author

lukashroch commented Oct 14, 2021

Yeah you're right. Though I'm guessing undefined or empty object will produce same result on request headers.

... There is also a question whether this is desirable to do so or design flaw. You could probably wipe lots of stuff on axios object. These could also be sealed so they can be modified but no removed.

@remcohaszing
Copy link
Contributor

remcohaszing commented Oct 14, 2021

In the readme modifying defaults is a documented feature, so I guess being able to delete header defaults is a feature as well.

@lukashroch
Copy link
Contributor Author

lukashroch commented Oct 14, 2021

I meant the internal implementation. If undefined or {} is producing same result on network request, then it's probably not much of a feature, it feels like it just adds to the the confusion that lots of it can actually be suddenly undefined. These props becoming undefined is probably more proper for current loosly checked implementation as axios can deal with it internally. But adds type overhead for TS users as the code could be more strictly typed ... if there is no benefit between two definitions).

@remcohaszing
Copy link
Contributor

remcohaszing commented Oct 14, 2021

I don’t know much about the internal implementations or the choices made there.

I don’t really have strong opnions about making the distinction between the header types for request configurations and axios defaults just to make the distinction between whether or not headers are optional. However, I do feel the types should reflect type correctness, which is what we’re both trying to solve here. :)

If you would like to make this distinction, I guess a second type needs to be added:

type DefaultsRequestHeaders = Record<string, string> | {
    common: Record<string, string>;
    delete: Record<string, string>;
    get: Record<string, string>;
    head: Record<string, string>;
    link?: Record<string, string>;
    options?: Record<string, string>;
    patch: Record<string, string>;
    post: Record<string, string>;
    purge?: Record<string, string>;
    put: Record<string, string>;
    unlink?: Record<string, string>;
}

@lukashroch
Copy link
Contributor Author

lukashroch commented Oct 14, 2021

Yeah, sounds alright. It's slightly off topic anyway. It's probably worth separate PR as instance creation headers should be optional regardless the decision regarding the default headers.

If your suggestion gets accepted by axios maintainers, this can be closed. If not, this should still be considered.

@Edmu
Copy link

Edmu commented Oct 15, 2021

Have created similar PR #4191

although things looks messy

var instance1 = axios.create({ headers: { common: { 'common-header-from-common': '1' }, post: { 'post-common-header': '1' }, 'common-header-from-outside-common': '1' }, });

`instance1.defaults.headers ->

common: {Accept: 'application/json, text/plain, /', common-header-from-common: '1'}
common-header-from-outside-common: "1"
delete: {}
get: {}
head: {}
patch: {Content-Type: 'application/x-www-form-urlencoded'}
post: {Content-Type: 'application/x-www-form-urlencoded', post-common-header: '1'}
put: {Content-Type: 'application/x-www-form-urlencoded'}
[[Prototype]]: Object
`

so if some common header gets put directly inside header object (rather that common) it does not goes to common but stays at headers level (although both are sent in request I think). So AxiosDefaults type is also not 100% correct, also it may be confusing for library user as user may thing that he always needs to delete header from common for it not to be sent...

@lukashroch
Copy link
Contributor Author

lukashroch commented Oct 15, 2021

yeah, @remcohaszing already pointed that out at the beginning of the thread. Juts haven't committed it yet.

@jasonsaayman jasonsaayman added component:typescript follow-up Issue or pull request requires a follow. labels May 4, 2022
@lukashroch
Copy link
Contributor Author

lukashroch commented May 4, 2022

@jasonsaayman any chance this could be looked at? It's been present in codebase for quite a while. And you can't create axios instances using factory function unless you ts-ignore it for now.

I've squashed and rebased it on top of the master and added ts tests. Happy to make further changes if needed.

@jasonsaayman
Copy link
Member

jasonsaayman commented May 5, 2022

For sure thanks, I am going to do everything relating to typescript in one go, there is a lot of it and the last time I made a large change a lot of people got super upset.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
follow-up Issue or pull request requires a follow.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants