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

Default headers support #206

Merged
merged 1 commit into from
May 8, 2018
Merged

Default headers support #206

merged 1 commit into from
May 8, 2018

Conversation

Kronuz
Copy link
Contributor

@Kronuz Kronuz commented May 3, 2018

This adds the feature in #205 by adding the following settings:

  • defaultAccept
  • defaultAcceptCharset
  • defaultAcceptDatetime
  • defaultAcceptEncoding
  • defaultAcceptLanguage
  • defaultAccessControlRequestHeaders
  • defaultAccessControlRequestMethod
  • defaultAuthorization
  • defaultCacheControl
  • defaultContentType
  • defaultCookie
  • defaultHost
  • defaultOrigin
  • defaultUserAgent

@Kronuz Kronuz force-pushed the master branch 3 times, most recently from a19989f to 5f29461 Compare May 3, 2018 18:39
@Huachao
Copy link
Owner

Huachao commented May 4, 2018

@Kronuz Thanks for your PR, and I am considering that if we could directly provide a setting like defaultHeaders. And user just needs to place all their expected headers into this setting, even custom headers can be set, not just restricted to the above given headers. What's your opinion?

@Kronuz
Copy link
Contributor Author

Kronuz commented May 4, 2018

That sounds also good... probably even better! Still, I’d get rid of defaultuseragent nonetheless, so there’s only one way to define that one.

One thing I stumbled upon while making the pull request is I had to create a new instance of the RestClientSettings in the HttpRequestParser, and I wasn’t very happy about it (as it adds new callbacks to and all)... is some other way you can think of to better access it from there?

@Huachao
Copy link
Owner

Huachao commented May 4, 2018

I am also considering this question that in current code, every place needs to reference the settings, they have to create a new instance of the RestClientSettings class. My suggestion is that, it seems there's no need to hold many instances of setting class, can we just leverage the singleton pattern. That means add a new public static function which is responsible for creating a setting instance internally if not exists, otherwise return the created one. And the constructor of it can be even made private.

@Kronuz Kronuz force-pushed the master branch 4 times, most recently from da8eb91 to 46d4f4f Compare May 7, 2018 20:30
@Kronuz
Copy link
Contributor Author

Kronuz commented May 7, 2018

@Huachao, I modified and added what you suggested to this pull request (defaultHeaders) 👍

@@ -83,8 +83,13 @@ export class HttpClient {
}

// add default user agent if not specified
Copy link
Owner

Choose a reason for hiding this comment

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

Please update the comment as well.

@Huachao
Copy link
Owner

Huachao commented May 8, 2018

@Kronuz please also update your code related to settings, since I have used the singleton pattern for Settings class. Thanks for your contribution.

@Kronuz
Copy link
Contributor Author

Kronuz commented May 8, 2018

@Huachao, done! I've changed it to use singleton settings and fixed the comment as well. 👍

@Huachao Huachao merged commit 679a93d into Huachao:master May 8, 2018
@Huachao
Copy link
Owner

Huachao commented May 8, 2018

@Kronuz Merged, Thanks!

@Kronuz
Copy link
Contributor Author

Kronuz commented May 8, 2018

Great! will you make a release any time soon? I can't wait to use the official extension again :)

@Huachao
Copy link
Owner

Huachao commented May 9, 2018

@Kronuz Sorry, the release will not happen in the immediate future. However I will notify you as soon as it happens

@Kronuz
Copy link
Contributor Author

Kronuz commented May 9, 2018

It’s ok, thank you! 👍

@Huachao
Copy link
Owner

Huachao commented Jun 28, 2018

@Kronuz you could test the latest version 0.19.0 to experience this feature, thanks for your contribution.

@aktxyz
Copy link

aktxyz commented Apr 7, 2022

Can variables be used in the settings file for defaultHeaders ??

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