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

Multiplatform OkHttp #6963

Closed
swankjesse opened this issue Dec 16, 2021 · 14 comments
Closed

Multiplatform OkHttp #6963

swankjesse opened this issue Dec 16, 2021 · 14 comments
Labels
enhancement Feature not a bug
Milestone

Comments

@swankjesse
Copy link
Collaborator

OkHttp has two roles:

  • An HTTP client API. Includes types like Request, Response, HttpUrl, and MediaType. Lots of visible design decisions here like whether a partial URL is representable, how much abstraction we layer on TLS, and what we include ourselves vs. delegate to JDK APIs.

  • An HTTP client engine. This is an implementation detail that performs the HTTP/1 and HTTP/2 calls. Lots of design decisions are hiding in here, including behavior of retries, timeouts, limits, and resource pooling.

When we shipped OkHttp 1.0 it was an engine only, borrowing HttpURLConnection as its API. We've implemented the Apache HTTP API on our engine. Ktor can also use OkHttp as its client engine.

And in this 2019 blog post, Uber described using OkHttp’s API with cronet as the engine.

Let's start with a multiplatform HTTP API only. We can facade existing engines on native and JS.

@swankjesse swankjesse added the enhancement Feature not a bug label Dec 16, 2021
@yschimke
Copy link
Collaborator

yschimke commented Dec 16, 2021

Should 5.0 of the KMM version be experimental? Likely to change in 5.1? Can we get it right in the first release?

I'm working on the assumption that this is still a strictly no breaking public API changes for JVM.

As discussed in Slack, it's a good opportunity to shrink some public APIs for classes, avoid anything deprecated in the JVM moving into common.

Complex types like X509Certificate deserve a lot of thought to get right and are exposed in very central types like Handshake.

@yschimke
Copy link
Collaborator

@swankjesse is 5.0 a good point to add a -kt module with await() suspending methods?

@swankjesse
Copy link
Collaborator Author

@yschimke with 5.0 we'll ship a tiny subset of our API as multiplatform, and expand coverage in subsequent releases. As long as we don't introduce new APIs we're unlikely to have regrets.

@yschimke
Copy link
Collaborator

yschimke commented Mar 4, 2022

This is probably complete enough to make a standard client call. (handshake isn't there, challenges are missing) But much of that is presumably provided by the native implementation.

@yschimke
Copy link
Collaborator

yschimke commented Mar 5, 2022

@swankjesse should ktor be our validating Client implementation? Will give us 100% coverage. But where to live? okhttp-ktor? Or a separate Block Inc. project?

@swankjesse
Copy link
Collaborator Author

We could use MockWebServer on the JVM to validate all platforms. Ktor won't really work cause it won't let us simulate errors.

@yschimke
Copy link
Collaborator

yschimke commented Mar 5, 2022

I meant client. Interesting idea using mockwebserver, can you run it from a jsTest without making it multiplatform? Basically what proves that an OkHttp 5.0 with multiplatform Call.Factory but no implementations is working?

@swankjesse
Copy link
Collaborator Author

Ahhhh... I think we should do our own direct binding to browser JS APIs.

For MockWebServer... we would connect to it via HTTP!

@yschimke
Copy link
Collaborator

yschimke commented Mar 5, 2022

But you don't have apis like MockResponse then.

@yschimke
Copy link
Collaborator

I think we have the API part in 5.0 now. Obviously work there is ongoing like IDN, but effectively working in typical cases.

@PaulKlauser
Copy link

Since KMP support was dropped in 5.0.0-alpha 13, should this issue be re-opened?

@swankjesse
Copy link
Collaborator Author

There’s still some KMP stuff I’d like to do. Shipping HttpUrl for KMP would be pretty rad.

@swankjesse
Copy link
Collaborator Author

#8578

@yschimke
Copy link
Collaborator

I think it's right to keep this closed, it isn't a broader aim. But +1 to letting the useful bits solve thorny problems for KMP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature not a bug
Projects
None yet
Development

No branches or pull requests

3 participants