Design document for the new HTTP API

[imiric]

This is a proposal/design document for a new implementation of the JavaScript HTTP API in k6.

The team has been discussing it internally for a few weeks, but the new API has been in discussion for many years prior to this. It's finally time to start working on it, so we want to share our ideas with the k6 community, and invite opinions from existing k6 users. Please be mindful to keep discussion thoughtful and relevant, in order to minimize noise.

[codecov-commenter]

Codecov Report

Merging #2971 (e58357f) into master (f893e83) will increase coverage by 0.06%.
The diff coverage is 86.41%.

❗ Current head e58357f differs from pull request most recent head 0fde824. Consider uploading reports for the commit 0fde824 to get more accurate results

@@            Coverage Diff             @@
##           master    #2971      +/-   ##
==========================================
+ Coverage   76.93%   77.00%   +0.06%     
==========================================
  Files         229      228       -1     
  Lines       16946    17059     +113     
==========================================
+ Hits        13038    13136      +98     
- Misses       3071     3080       +9     
- Partials      837      843       +6     

Flag	Coverage Δ
ubuntu	77.00% <86.41%> (+0.14%)	⬆️
windows	?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
api/v1/client/client.go	0.00% <0.00%> (ø)
cmd/run.go	72.76% <ø> (ø)
cmd/test_load.go	79.71% <ø> (ø)
js/common/bridge.go	100.00% <ø> (ø)
js/common/initenv.go	66.66% <ø> (-33.34%)	⬇️
js/console.go	94.11% <ø> (ø)
js/modules/modules.go	33.33% <ø> (ø)
js/summary.go	89.87% <ø> (ø)
lib/execution_segment.go	92.73% <ø> (ø)
lib/executor/constant_arrival_rate.go	96.68% <ø> (ø)
... and 49 more

... and 8 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[codebien]

Do we have an issue with it? It sounds like a prerequisite for the 3rd phase.

[imiric]

This was my attempt at coming up with a File API, and is more of a thought experiment than anything else. #2977 goes into much more detail, and these kinds of issues should be addressed there.

This is inspired by Deno's API. While they do have sync versions of most APIs, it's clear that if we want to adopt streams, we'll need to defer actual reading of files to whatever process needs it. I.e. we can't read the whole file into memory here in the init context, so it's likely that this will open a file handle only, and file.readable will be the stream reference. Whether that can be done without await, or if we'll need to support this in the init context, is an open question.

[mstoykov]

I thought I commented this ... but :(

(top level await](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/await#top_level_await) is a ES2022 feature that made all the module resolutions in ESM asynchrnous. Which is partly what makes them a bit more involved and is what blocked them on async/await support in goja.

As soon as ESM is merged this will work as well. While I am in practice blocked specifically on fixing basically this functionality - I do need it for the basic implementation, so ... yeah - once ESM is native it will work.

2 workarounds are possible until then:

1. we can make the already existing wrapper async - this likely will have ... strange behaviours, but we already need to fix those around ESM discussion: open and require are relative to? #2674
2. The workaround for before this was a thing was to make an inline async lambda and call it. This has some usability problems, but will likely be okay for a release or two, given that I expect this functionality will not be released before ESM is on the final stretch.

[codebien]

If we implement fetch as a JS polyfill can we combine it in a native module? I guess we could have a wrapper on top of the new-k6-http-client+the polyfill. Am I missing something?

I think this is a nice UX and if we would not able to do it then we should insert it as a risk.

[imiric]

My thinking so far is that the Fetch API would be part of the native Go module. There's really no reason it needs to be a JS library, and it would be a small addition anyway, so it shouldn't matter.

[codebien]

If they are confirmed, do you plan to include the concrete proposal for them in this document?

[imiric]

#936 would be fixed by the version property.

h2c could be supported via another property, so maybe:

  const client = new Client({
    version: 2,
    insecure: true,
  });

Though this would mean that you couldn't pass an already connected socket, or it would force a reconnection. 🤔 Not sure, we need to decide how to handle the whole Socket/Transport API, and if we're implementing that first, or exposing it later. Once we have a better idea of what we're doing there, making h2c configurable would be a relatively minor addition.

But to answer your question: yes, we should have proposals for issues we plan to work on in phase 1 before merging this PR.

[na--]

From what I know, HTTP version negotiation happens at a lower level, so I am not sure if the Client is the place where we should have a version property 🤔 On the network or transport level is probably more appropriate 🤔 Though I am not sure if we should even specify these things in this design doc, a PoC seems like a better place to figure them out

[imiric]

HTTP version negotiation happens at a lower level

Right, it can happen during TLS negotiation, as part of ALPN. But HTTP/1.1 connections can also be upgraded via a header. I'm conflicted about it as well, though it definitely should be somewhere lower level as well.

Maybe part of TLS.connect()?

import { TLS } from 'k6/x/net';
const tlsSocket = await TLS.connect('10.0.0.10:443',
  { alpn: [ 'h2', 'http/1.1' ] });
const client = new Client({
  socket: tlsSocket,
});

This way you could force HTTP/1.1 over TLS, even if the server supports HTTP/2. This could be simplified to a versions array, instead of alpn.

Since h2c must be negotiated via an Upgrade header, and can only be done without TLS, then something like the insecure flag above on the Client itself would be the way the go, in which case we should also keep the version property. It wouldn't make sense to specify that as a, say, TCP.open() option.

But I agree that we don't need to agree on every single detail to start working on the PoC. We can iron out these details as we make progress.

[mstoykov]

Just to note that the UPGRADE header is only used for (in practice) websockets and h2c without "prior knowledge".

The usuall http2 upgrade happens in practice in the tls handshake and h2c with prior knowledge just starts talking http2 directly.

http3 being over UDP means that the upgrade is basically a completely new connection. Except again if "prior knowlege" is used in which case it is still on a new connection, it just skips the first one ;).

I am not certain where this should be configured ... or even if it should be one place or if it should be solved with some more involved setup.

[codebien]

Suggested change

	Good candidates: [#936](https://github.com/grafana/k6/issues/936), [#970](https://github.com/grafana/k6/issues/970).
	This initial API must solve a minor, but concrete, issues of the current API. It fixes something that's currently not possible and doesn't have a good workaround as [#936](https://github.com/grafana/k6/issues/936) and [#970](https://github.com/grafana/k6/issues/970).

I think we can promote them.

[imiric]

@codebien See 81034aa.

[imiric]

In 0fde824, I added TLS options to TCP.open(), instead of having TLS be a separate k6/x/net class.

[na--]

This and the tentative HTTP/3 example below don't look very realistic, or even very user-friendly 🤔 There are a couple of missing links here, we generally don't go from an TCP/UDP connection directly to an HTTP client.

We want something (i.e. another component) to make network connections, not to have to make them raw. And we want something (also another component or a configurable part of one) to be able to negotiate which HTTP version to be used, or to be able to force that a specific HTTP version is used. Controlling these things by requiring users to make raw TCP/UDP connections doesn't seem like the way to go, especially given how complex HTTP protocol version negotiation and how it's intermingled with DNS and TLS...

[imiric]

True, conceptually there's a missing "transport" abstraction between raw sockets and the high-level HTTP client. But this is an example of how both sides can be integrated when the user needs to configure socket/transport-level options. The friendlier example is the one above this, where you simply instantiate a Client, and make requests with it, using the default transport settings.

we generally don't go from an TCP/UDP connection directly to an HTTP client

You can certainly talk HTTP/1 over a raw TCP socket. HTTP/2+ and TLS complicate this substantially, but ideally we would like to keep a friendly API for all versions, without introducing too many concepts.

The reason I'm hesitant to expose yet another abstraction is because the API would be even more confusing. A "transport" is uniquely a Go concept; no other JS runtime that I could find exposes it in the way Go does. For example, Deno has Deno.connect() and Deno.connectTls(). Node has entirely separate net and tls modules. That's not to say that we can't do something entirely different, but it would go against our intention of building something familiar to JS users.

That said, the following makes sense to me:

• A raw Sockets API to read/write TCP/UDP/IPC directly. Since host name resolution must happen before a connection is established, or higher level protocols are negotiated, a lookup hook as shown in the current examples is a good way to configure the DNS logic. This is inspired by Node.

• An optional Transport API that wraps the raw socket, and implements whatever protocol or security on top. These implementations must implement the same Socket interface, so that the http.Client can be unaware whether it's using a raw socket connection, or e.g. a TLS connection. This would mimic Node, where TLSSocket extends the base Socket, so that it can be used transparently.

  So, for example, TLS options could be specified like this:

  import { TLS } from 'k6/x/net';
  const tlsSocket = await TLS.connect('10.0.0.10:443',
    { version: { min: 'ssl3.0', max: 'tls1.3'} });
  const client = new Client({
    socket: tlsSocket,
  });

• The high-level Client API would do only HTTP-specific negotiation. So if HTTP version 2 is requested, but a non-TLS connection is passed to it, it would throw an error. For that to work, an insecure: true option must be passed, which would negotiate h2c.

Let me know if you disagree with anything above.

[na--]

I fundamentally disagree with a lot of it, mostly because I don't think it will work at all, and even the parts that might work won't work well. Even if they worked, raw sockets are probably going to be the wrong abstraction 99% of the time, for anything the user may want to do with custom HTTP settings. I don't dismiss we should have support for them (though I am not sure if we should start with that), I just think there should be a layer or two between them and the HTTP Client.

This is why I don't think they will even work for most things. Consider even the example you provided:

const tlsSocket = await TLS.connect('10.0.0.10:443',
  { version: { min: 'ssl3.0', max: 'tls1.3'} });
const client = new Client({
  socket: tlsSocket,
});

I see at least 4 separate issues with this small piece of code:

1. Most of the time, you need DNS to establish TLS, right? Because the certificates are for the domain names.
2. You need to negotiate the HTTP/2 protocol version during the TLS connection establishment, so we can't easily split the TLS connection and the HTTP requests... See https://en.wikipedia.org/wiki/Application-Layer_Protocol_Negotiation
3. What happens to connection reuse? What happens if we don't want that and want a new connection for every request. It's perfectly reasonable for a single HTTP client to make multiple (potentially simultaneous) network requests to the same or to different hosts.
4. What happens if we have a redirect and we want the Client to follow it? 😅

In short, an HTTP Client is something that needs to be able to make its own network requests through some mechanism. You can't give it a single Socket and expect it could work just on that

[imiric]

I fundamentally disagree with a lot of it, mostly because I don't think it will work at all, and even the parts that might work won't work well.

That sounds like a complete disagreement about everything I've said. 😓 I'm trying to find some common ground we can make progress on, but it sounds like you want us to go back to the drawing board with most of it. Please try to have an open mind about things that could work, and propose concrete suggestions to improve things that you think might not. It's exhausting trying to come up with an API design to address all the issues we want to fix, and then get fundamental disagreements about it. Particularly because all of this is entirely abstract at this point, nothing is set in stone, and the current proposals are just ideas. We shouldn't have such strong disagreements this early on in the process when so much is still unclear...

I'll try to address your concerns:

1. Most of the time, you need DNS to establish TLS, right? Because the certificates are for the domain names.

Right. Did you miss the lookup hook from the TCP example? The TLS.connect() method could have the same mechanism. This is inspired by Node.

2. You need to negotiate the HTTP/2 protocol version during the TLS connection establishment, so we can't easily split the TLS connection and the HTTP requests

Yes, I made a suggestion here for that.

3. What happens to connection reuse?

4. What happens if we have a redirect and we want the Client to follow it?

The HTTP client will be able to establish connections itself as needed. See the simple example where only a Client instance is used. This passing of an already established connection is done to configure the initial connection, which the client will use instead of creating a new one.

Currently it's not clear how other connections could be configured. Could the same settings from the initial connection be reused? How about via an event handler that is called before establishing a connection, or before processing a redirect? I've yet to receive a comment about the event system, which I think could address some of the advanced scenarios in an idiomatic JS approach.

We can replicate Go's API in JS and call it a day, but as we (I think) already agreed, we want to build something that's familiar to JS developers, while being simple and intuitive to use. This is why I'd like to avoid introducing more abstractions, especially inspired by Go, since we're all biased by it.

Can you propose a solution to your concerns that aligns with this? It would be quicker to see exactly what you have in mind, rather than spend more time in a back-and-forth.

[na--]

Let me focus just on one thing then:

This passing of an already established connection is done to configure the initial connection, which the client will use instead of creating a new one.

To me this seems like a fundamentally flawed approach which can't work. It also unfortunately seems to be somewhat at the core of your proposal for handling some of the lower-level connection details.

I already explained some of the reasons why I don't think it will work. I also don't remember ever seeing an approach like that, in any of the languages or libraries I've used to make HTTP requests. I don't see such an approach in any of the links you provided either:

• node's http.request() has a createConnection() callback
• deno seems to use Fetch, which is limited, but you can create a new HTTP client with custom options
• axios seems to allow you to define the transport and proxy in every request

I may be missing something, so please share at least one example from any language or HTTP library where something like this is done, to convince me that it's a viable path forward.

It's good to have low-level network connection APIs. It's fine to somehow even allow their usage from the HTTP Client (e.g. by specifying a callback function that can make and return custom connections). But an HTTP client doesn't need a single network connection, it needs a mechanism to make connections, a connection factory 😅

[imiric]

I'm not claiming that what's currently here is a viable path forward. I'm expecting it will need many changes, and this is why we're discussing it. I'd like this document to be a team collaboration, rather than a traditional PR review. If you notice, the proposal author is "The k6 core team". 🙂

I'm also not disagreeing with your feedback, for the most part. Some of the points you brought up are also unclear to me. So I can try to come up with a design that addresses those issues as well, and then wait more time for other concerns to pop up, or you can just suggest concrete improvements here, or—even better—push changes directly to this branch. This is not "my proposal".

This Socket <-> HTTP integration is probably my idea, so I don't have an example from similar APIs. What I was trying to avoid is the complexity of Node (notice that they also have the concept of an Agent that manages connections), and Deno doesn't really have the flexibility we need, since its HttpClient is very limited, and is mostly a bag of options that are passed to fetch(). I haven't looked at Axios yet, but from a glance, that transport option is very unclear. 😕

I think we can either handle this via some kind of factory hook, similar to Node's createConnection(), or via an event that can be emitted before creating the connection. I'm leaning towards events, as those can be applied for any advanced scenario, and are idiomatic to JS.

What I wouldn't want is to have a bunch of new abstractions, where we have to explain concepts like transport, dialer, agent, or whatever else.

[na--]

I'd like this document to be a team collaboration, rather than a traditional PR review. If you notice, the proposal author is "The k6 core team".
This is not "my proposal".

Fair points and I am sorry! After re-reading my comments from yesterday I realize they were probably a bit too direct... 😞 I apologize, I didn't mean to offend or be personal, just to comment on why I thought the specific technical proposal was infeasible.

I think we can either handle this via some kind of factory hook, similar to Node's createConnection(), or via an event that can be emitted before creating the connection. I'm leaning towards events, as those can be applied for any advanced scenario, and are idiomatic to JS.

Both of these sound like viable approaches to me. Event hooks also seem more idiomatic to me, in general, but I am not sure if they are suitable for this type of work 🤔 They are usually useful for notifying the user before or after something happens, to potentially give them the option to modify the request or response, but to make a completely different connection it seems to me that we'd be better of with some sort of a callback.

What I wouldn't want is to have a bunch of new abstractions, where we have to explain concepts like transport, dialer, agent, or whatever else.

Not sure I agree about this part, but I am open to be convinced. Having a bunch of small and self-contained "abstractions" that work well together seems like a simpler approach than having one big HTTP Client thing with a bunch of convenient ad-hoc hooks and callbacks. We shouldn't consider just the new HTTP API in complete isolation.

For example, if we have a "dialer" (or whatever we call a similar concept) that supports a SOCKS proxy, it can work with the HTTP client, but we should also be able to reuse it for gRPC or WebSockets or pretty much any other protocol with a bit of work. Same for a DNS resolver, or for TLS authentication with client-side certificates, etc.

Mind you, these "abstractions" might actually just be regular JS functions with well-defined APIs (e.g. "I give you an address, you return an IP", or "I give you an address, you return me a network connection"). This seems like an idiomatic JavaScript way to implement something like this. But, to be composable, they need to be clear and distinct and reusable.

[imiric]

I added a dial hook to the Client constructor in 0fde824, along with TLS options to TCP.open().

I think this addresses what we discussed above, otherwise please suggest alternatives.

[na--]

This part of the current HTTP API is probably not a good design to copy. Why should a generic HTTP response have a .json() method?

It's fine to have something that wraps a regular Client and works only with JSON, but a generic http.Client should probably not need to know what JSON is.

[imiric]

http.Client wouldn't know what JSON is, but I don't see why http.Response couldn't have helper methods to parse the response body and return it as various objects.

The web Fetch API has a Response with such methods, and so does Deno's implementation. Regardless if we end up implementing Fetch or not, this is very convenient, and it would be a UX regression if we decide to remove them.

[na--]

Fetch and Deno might have this, but Go, axios and got don't. We should consider what makes sense for us.

These are my thoughts on the topic, in roughly a descending order of certainty:

1. We definitely shouldn't have a .json([selector]) method like the current one, where we have a selector as the argument. That should be a separate JSONPath API that is not tied directly together to the core HTTP API, it should probably work with any string or buffer.
2. It's fine to have a Fetch polyfill/wrapper that has a .json() method, built on top of our generic HTTP Client (either in Go or in JS!). This solves some of the UX problem of a generic API not having one.
3. It's probably a good idea to have a dedicated custom JSONClient type built on top of the generic Client, which will be well suited for easily working with JSON REST APIs. Including potentially automatically marshaling request bodies that are objects and maybe even automatically universalizing responses. Maybe even with some JSONPath integration 🤔 This provides all of the UX benefits, but without anything magical and with a clear separation of concerns (i.e. the composable approach).
4. The default generic HTTP Client should not make any guesses about the content of the HTTP requests and responses it is handling. This gives us more room for optimization, a clear separation of concerns, and a more consistent UX.
5. We probably shouldn't have a .json() method (without any arguments) on the generic HTTP Client. If someone really wants to use the generic k6 http Client for one-off request with a JSON response, then JSON.parse(resp.body) is not that much worse than resp.json()...
6. If we decide to add .json() , are we also going to add to port the current .html()? If not, then why not? And are also going to have .xml() or .protobuf()?

[imiric]

Fetch and Deno might have this, but Go, axios and got don't.

I wasn't familiar with got, looks nice 👍 It doesn't implement json() directly on Response, but does on the Promise. It even supports the json property for POST requests.

BTW, there's another HTTP lib from the same team called ky. 😅 This one apparently targets Deno, but it follows similar design principles as got.

Axios actually parses JSON bodies by default 😄 But I don't think we should look at it for inspiration, considering it's quite old at this point, and has been superseeded by other libs. It's still very popular and featureful, but also has many limitations.

Go shouldn't be an inspiration for the JS API. As stated in the design goals, we want to make things familiar to JS devs, not Go devs. We can borrow certain concepts to make the API composable and whatnot, but refering to it directly would only pollute our way of thinking about idiomatic JS.

We should consider what makes sense for us.

If by "us" you mean "k6 users", then agreed. These convenience methods are all part of offering a good experience for JS developers. I don't see why that's so controversial.

To address some of your points:

• Sure, the Selector API makes sense as a separate JSONPath API. json() should just return the body as an object, as it does in the other JS libs.

• Having purpose-built Client implementations just for sending different headers, interpreting requests and responses, seems like overkill, and might be too limiting in some cases. What if you want to send JSON but receive binary data, or any such weird combination we didn't predict? Having a single Client that offers some helper mechanisms for the most common use cases adds a fairly low overhead, which can be ignored for those who don't need it. I.e. everyone can just ignore the json property, and use body instead.

• I'm not sure why you keep mentioning this, but Client itself wouldn't have a json() method. It would return a Promise<Response> which would have it.

• I don't see why we couldn't have html() either. The point of these methods are convenience for the most common use cases. If there are good reasons to include xml() or protobuf(), then we should consider them as well.

  Are you suggesting that we have an HTMLClient, a ProtobufClient or an XMLClient instead?

  Separate client implementations make sense for truly different protocols. So it probably makes sense to have a GRPCClient that extends the base Client, or maybe a SOAPClient 😄, but not just to avoid these simple payload handling helpers.

[na--]

Go shouldn't be an inspiration for the JS API. As stated in the design goals, we want to make things familiar to JS devs, not Go devs. We can borrow certain concepts to make the API composable and whatnot, but refering to it directly would only pollute our way of thinking about idiomatic JS.

I mostly agree, but I will push back slightly on this. Go can and probably should be an inspiration, as long as the end result is also an idiomatic JavaScript API. If Go has an elegant solution to a problem, and if that solution doesn't look strange for JS APIs, is easy to understand, and cleanly solves a bunch of problems, why not adopt it? 😕 Sure, we don't need to copy the Go API directly, but we don't have to avoid its good ideas just be cause they are not JS. Good API design is somewhat universal after all...

Having purpose-built Client implementations just for sending different headers, interpreting requests and responses, seems like overkill, and might be too limiting in some cases.

I didn't meed completely separate Client implementations. Just wrappers around a generic Client with maybe a few extra methods and some pre-defined Event handlers for the same events that you mention in the design doc.

What if you want to send JSON but receive binary data, or any such weird combination we didn't predict?

Use the generic Client, if it's a one-off, or build your own wrapper (because you should be able to easily do that, since everything is composable).

Having a single Client that offers some helper mechanisms for the most common use cases adds a fairly low overhead, which can be ignored for those who don't need it. I.e. everyone can just ignore the json property, and use body instead.

Sure, but having more than one way to do the same thing, especially when it's a simple thing, is generally somewhat of an API design smell. I don't object too strongly to the simple .json() method, it's only at point 5 in my list, but I don't see the need. It's also one of these extra things that we can easily add at any future point, but once we add it we can never remove, so I don't see the need to start with it.

I'm not sure why you keep mentioning this, but Client itself wouldn't have a json() method. It would return a Promise which would have it.

Sorry, I meant the Response object returned by the generic Client (or, I guess, the Promise that resolves to it).

I don't see why we couldn't have html() either. The point of these methods are convenience for the most common use cases. If there are good reasons to include xml() or protobuf(), then we should consider them as well.

See the problems that .html() causes for the current k6/http API... Tightly coupling these is a mistake.

Are you suggesting that we have an HTMLClient, a ProtobufClient or an XMLClient instead?

No, that was the point. At best, we may have some helpers and wrappers, but we probably shouldn't. We should make it super easy and efficient for users to build whatever helpers and wrappers. We should provide the flexible and generic toolbox, not try to build a dedicated tool to solve every possible weird use case that exists out there.

[na--]

Hmm actually, I am slightly walking back some of my claims... 😅 Thinking about idiomatic Promise-based JS APIs, a .json() helper on a Promise that also returns another Promise actually makes some sense 😅 I still don't like it, but it probably provides enough value to pass muster 🤔

[na--]

Same logic as https://github.com/grafana/k6/pull/2971/files#r1161730930 - we shouldn't add any special handling of json options in the default Client. This is not composable, it's the opposite of that - trying to make a Client that satisfies all use cases. And it adds "automagic behavior", as your // automatically adds 'Content-Type: application/json' header comment directly shows.

The request bodies that the normal HTTP Client accepts should probably be limited to string, ArrayBuffer or some sort of a Stream. Anything else needs to be an error. We can have a separate RESTClient or JSONClient that has additional automated handling of JSON requests and responses.

[imiric]

This is not really "automagic" behavior, but a convenience for a very common use case. It's inspired by the request library, and was suggested in #436. The json property makes it clear what happens behind the scenes, and implicitly adding the header is much more convenient than having to remember to type it correctly manually, and using JSON.stringify().

I'll remove this if you feel strongly about it, but having separate client implementations just for this seems overkill.

[na--]

The problem is that it makes the API have weird edge cases. For example, what happens if I provide both body and json and formData? 😅 Now the library needs to have some extra internal logic for the order of precedence, and we have do document and test that, etc. Having more than one way to do the same thing in the same object is generally not great API design.

[na--]

Actually, from looking at the request library API you linked to, its json property is a boolean one, you still have to supply to body, right? Which may be even worse than this suggestion, since you now have extra parameters that just control the behavior of how the body is automagically processed.

[imiric]

Yes, it slightly complicates the validation, but passing conflicting options should just error out.

No, json can either be a boolean or an object.

json - sets body to JSON representation of value and adds Content-type: application/json header. Additionally, parses the response body as JSON.

I don't like the boolean functionality either, but serializing the passed object and adding the header makes a lot of sense. This is also supported by got, as I mentioned in the other thread.

[na--]

This API proposal seems a bit messy. The HTTP request method should probably be part of the Request object, right? The whole point of a Request object is to fully contain everything you need to make a request.

But then, what client.get() probably shouldn't accept Request parameters, since that Request could be a POST. I am not sure if methods like .get() and .post() are even necessary in the default HTTP Client API 🤔 Maybe a single generic Client.request(Request) or Client.do(Request) is enough. But if we choose to have helper methods, they probably shouldn't accept a Request, just a method, params, body or something like that 🤔

[imiric]

This example is intentionally contrived, to show how request options can be applied globally and then merged/overridden, either in a Request object, or for each individual client.get() call.

method should indeed be part of the Request object. Essentially, most options that can be passed to client.{get,request,...}, can be used to construct a Request object. The point of Request is to reuse common options to make multiple requests.

I think overriding should be done from the bottom-up (or top-down, depending on how you look at it 😄). I.e. the options passed to client.{get,request,...} will override whatever was set in Request, which will in turn override whatever was set globally. So if you set method: 'POST' in the Request, and you call client.get() with it, then a GET request will be done, with whatever else was in the Request object.

The same confusion could arise if we only allow Client.request(Request). What should happen if you have:

const r = new Request('https://k6.io/', { method: 'GET' });
client.request(r, { method: 'POST' });

?

Removing the helper .get() and .post() methods would just remove the convenience to not specify method everytime, but not this possible inconsistency.

Or would you want to remove all options from client.request() as well, and only allow passing a Request object? 🤔 I think forcing the use of Request always would be inconvenient.

[na--]

👍 for most of the text here, e.g. I agree with the "Why is this needed?" section, the rough "Implementation" plan, and the text at the start of the "Proposed solution(s) -> Design" section.

However, looking over the JS API examples, I see a lot of potential issues. I noted some of the bigger ones as inline comments. We probably shouldn't (and realistically can't) figure out all of these details in this design document, those would probably be better off to leave for when we are iterating on the implementation, so we don't necessarily even need to fully fix them here. But at least we should agree on some rough guidelines (e.g. when it comes to automatic JSON handling or how to negotiate/fix HTTP versions).

[mstoykov]

I would argue the proposal here is about making things possible.

Both we and every user can (and will) extend this API.

But for me adding a bunch of UX improvements and that being not a small part of the specification does not help with the discussion around - it hinders it. I now or whoever has to read a bunch more text and then decide that whoever wrote it and the people who have agreed, meant those as inspirational things instead of as goals that need to be reached by this proposal to be called fulfilled.

[mstoykov]

edit: sorry for the slow reply, I kept coming back here and reading the other discussions instead of replying to the old ones. And github is really bad at in practice showing discussions in a way that doesn't require you to scroll 20 screens to see a comment you want to reference.

As I mentioned in the other comment, the Fetch API will be a very minor addition in later phases of development. I still think it's worth mentioning as a general design goal that people can look forward to. On its own, I don't think it deserves to have a separate design document, but it can live as a bullet point here.

That is great, but if it is in this proposal we need to discuss it and agree that we want it as part of the proposal. And I am not certain I would like to have the fetch API implemented. This also goes for all the other things that you argue are "general deisgn goals that people can look forward to". If its in the proposal, we need to agree on it with the proposal. If its in an issue that references the proposal as a requirement - it doesn't need to be agreed with the proposal.

I am not saying that you should open an issues or making new proposals. But the idea of the proposal is that we agree on what we want to do and how we are going to try to do it. And I (and maybe others) aren't certain that fetch or per HTTP verb methods are a good thing. I certainly don't think they should be part of this proposal.

If you're referring to the Sockets and DNS APIs, I agree that it expands the scope outside of what a purely HTTP API should be concerned about, but I think it's important to think about them early on, even if we don't expose them to JS. This will help us structure the implementation and Go API in such a way that makes it easy to eventually expose them to JS. I think this is the case for the Sockets API, at least, while the DNS API can be implemented later.

I am not against discussing them. The oppossite, but I would like to have some clarity on what will be implemetned as part of this proposal and what not. As it seems that to be part of the proposal idea - stating what we will do.

We can always extract them to standalone design documents, and reference them here, but if we don't think about them early on then we run the risk of creating an API similar to the current k6/http, which does many things internally and makes it difficult to later refactor and expose.

Again I do agree we should think about them 👍

Is it "tiring to read" because of the amount of information and the content itself, or because you don't see them mentioned in the Implementation section? Please suggest what would make the content less tiring to read, as that's not desirable. The reason some of it is not mentioned in the Implementation section is because we haven't decided how/when/if to approach it, which is what the TODO in phase 3 is partly for. If we decide to work on the Sockets API early on, then we should mention it in phase 1 or 2, and DNS API in phase 3, etc. So let's make a decision.

The tiring part is that I don't know why I am reading it and where it goes. And then when I get to the end, I am "why did I read this" as I never got to see when they will be implemented.

So I guess I am proposing that part of the proposals are more accurately described as ... ideas (?) on how to implement the API on top.

Personally, I think we should start from the bottom up, so implement the Sockets API first, and then build the HTTP API on top of it. We might decide to not expose the Sockets API to JS yet, but as I said above, I think it's important if we think about it from the start.

I am okay with that but arguably that should be stated in the proposal. When I last read it in full it very much sounded like you intend on implementing Client. And then (at some point) expose the underlying mechanisms (Dialer, Socket, Transport ... or w/e else) to JS for more control over different parts.

[imiric]

Reproducing this comment here for visibility:

Would it help if we split this large proposal into several smaller ones? The scope is quite large already, and we haven't even fleshed out the details of the Sockets or DNS APIs. Splitting this into separate proposals would make each one more manageable, and allow us to prioritize them as a related group. Then the UX improvements, the Fetch API and any such convenience wrappers, could go into one.

I'm not sure where to start with this, so if we agree to do this, any suggestions to move it forward would be appreciated.

[codebien]

Suggested change

	Good candidates: [#936](https://github.com/grafana/k6/issues/936), [#970](https://github.com/grafana/k6/issues/970).
	This initial API must solve a minor, but concrete, issues of the current API. It fixes something that's currently not possible and doesn't have a good workaround as [#936](https://github.com/grafana/k6/issues/936) and [#970](https://github.com/grafana/k6/issues/970).

I think we can promote them.

[codebien]

I'm fine with the current version and start iterating on the first phase. I think after it we will have a lot of acquired knowledge to report back and to have a more accurate and final version of this document.

[mstoykov]

I approve this with the idea that while we are very likely to not do most of the stuff in this proposal as is.

But I also do expect that somewhere along the way it will turn out a bunch of the stuff proposed will not be enough.

I also have to again point out that at least some of the (later) functionality is based on functionality we currently don't have good support such as streams and events (which arguably can be implemetned on case by case bases).

So I do expect this will hit a bunch of issues a long the way and likely will need to be heavily refactored/rewritten as things arise. And that is okay with me.

[imiric]

I approve this with the idea that while we are very likely to not do most of the stuff in this proposal as is.

But I also do expect that somewhere along the way it will turn out a bunch of the stuff proposed will not be enough.

I also expect many changes to this document as we make progress, but I'm not sure how you can be so certain that we're very likely to not do most of this as is. The design here is abstract at this point, but most of it is grounded in APIs from other JS runtimes. If you have strong objections to the practicality of any of it, then please propose alternatives we can discuss (in a new PR 😄), but it's not helpful to be generally dismissive about the work we've done so far.

Thanks for reviewing! 🙇