Refactor HttpClient to its own class #29
Conversation
Which makes its headers fully configurable by the Inngest client. It should also be able to pool network connections correctly given that it's the same instance. According to: https://square.github.io/okhttp/contribute/concurrency/#connection-pool
KiKoS0
requested review from
djfarrelly,
darwin67,
jpwilliams and
tonyhb
as code owners
| } | ||
| class Inngest | ||
| @JvmOverloads | ||
| constructor(val appId: String, framework: String? = null) { |
There was a problem hiding this comment.
Framework needs to be inferred from the adapter and is never a value passed in on the constructor.
There was a problem hiding this comment.
makes sense, I'll address it 👍
|
Or I can change it later as well, so you can merge as is if you want |
It's now set to the `CommHandler` similarly to the other SDKs.
| InngestHeaderKey.Sdk.value to sdk, | ||
| InngestHeaderKey.UserAgent.value to sdk, | ||
| InngestHeaderKey.Framework.value to (framework?.value), | ||
| ).filterValues { (it is String) }.entries.associate { (k, v) -> k to v!! } |
There was a problem hiding this comment.
is this to check for null values? Why is SupportedFrameworkName nullable anyway, could we make it not nullable?
There was a problem hiding this comment.
it's nullable because the Inngest's client uses it and isn't provided the framework option as mentioned ☝️ by Darwin: inngest.com/docs/reference/client/create.
also looking at these two header generation functions in the other sdks, i think we'll have more optional headers implemented:
https://github.com/inngest/inngest-py/blob/2dd0ebd45c2fdbb035c03e0ec188567a54e76c31/inngest/_internal/client_lib.py#L137-L144
https://github.com/inngest/inngest-js/blob/1b9f101ca6cd310e429cebcf8bceaf2faf309624/packages/inngest/src/helpers/env.ts#L297-L301
There was a problem hiding this comment.
fair enough, I guess it's possible people will want to use the Inngest client directly without a framework adapter
| ): T? { | ||
| val request = httpClient.build(url, payload) | ||
|
|
||
| return httpClient.send(request) lambda@{ response -> |
There was a problem hiding this comment.
There was a problem hiding this comment.
I wonder if it's possible to restructure the SDK to use these to manipulate control flow instead of exceptions. We can consider it later since things work as is
Refactors API calls into a separate
HttpClientclass.Making its headers fully configurable by the
Inngestclient.It should also be able to pool network connections correctly given
that it's the same instance. According to: https://square.github.io/okhttp/contribute/concurrency/#connection-pool