feat: support for custom http headers in RpcClient #861
Conversation
| @@ -17,8 +17,18 @@ abstract class RpcClient { | |||
| factory RpcClient( | |||
| String url, { | |||
| Duration timeout = const Duration(seconds: 30), | |||
| Map<String, String> customHeaders = const {}, | |||
| bool includeDefaultHeaders = true, | |||
There was a problem hiding this comment.
Is there any use-case when the default header Content-Type should not be included?
There was a problem hiding this comment.
Yes, thats actually the reason for this PR. The default Content-Type header has ;charset=utf-8 included. I recently came across a solana RPC endpoint implementation that strictly requires application\json (see extrnode/extrnode#1).
(Side note: Unfortunately, this PR will still not make me able to use extrnode, sine there is a bug in the dart http implementation itself, see dart-lang/http#184 and my PR dart-lang/http#886)
There was a problem hiding this comment.
I see, but you still need to include this header, you just need to alter it, right?
I suggest adding just customHeader parameter, and then using it like that in constructor:
headers: {..._defaultHeaders, ...customHeaders},There was a problem hiding this comment.
I personally am fine with that.
But just to clarify my intention: The idea behind that bool is to make it transparent to the users of the library that there might be other headers added by the library itself. I wanted to prevent a scenario where a user does not set a value in customHeaders and then starts wondering why outgoing requests contain something he has not added.
There was a problem hiding this comment.
In general, the library should not add any headers that are not absolutely necessary (and in that case, user shouldn't remove them). Content-Type should be there anyway, but I would actually remove charset from the library default header. The official node seems to be working fine without this parameter.
And then, with the possibility to override the default header, you can still change it if needed.
There was a problem hiding this comment.
I've implemented your suggestions 😃
|
@EPNW-Eric can you please merge with the latest master branch? We've updated dependencies, so actions should run successfully now. |
Codecov Report
@@ Coverage Diff @@
## master #861 +/- ##
==========================================
+ Coverage 79.00% 79.15% +0.14%
==========================================
Files 181 181
Lines 3916 3933 +17
==========================================
+ Hits 3094 3113 +19
+ Misses 822 820 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
It would be nice if we could add custom http headers to the RpcClient, and even override the default ones, so I added this very basic functionality.