fauna-js icon indicating copy to clipboard operation
fauna-js copied to clipboard

Organize request and response types

Open ptpaterson opened this issue 2 years ago • 2 comments
trafficstars

Ticket(s): FE-3105

This PR shuffles types about how I have come to view them. I hope that it can be a good step toward clarifying the mental model for users of the driver.

Problem

  • Request and response types can be cleanly separated into different modules
  • QueryRequest extends QueryRequestHeaders, but QueryRequestHeaders fields are not actually sent over the wire, which muddies the types.

Solution

Separate QueryRequest and QueryRequestHeaders so we can enforce that only a QueryRequest provided to the HTTPClient and thus over the wire. Move QueryRequestHeaders out of the wire-protocol module.

Result

Separate types into multiple files

Created separate files for request and response types. They are still in a folder called `wire-protocol' so imports don't actually change.

Query Options

QueryRequestHeaders was also renamed to QueryRequestOptions to reflect that they could be any options (though they happen to currently only include specific header values).

Instead of duplicate the same fields in ClientConfiguration, moved those into an embedded, optional queryOptions field that is a QueryRequestOptions object. I documented that these are the options sent be default for each query. This way, we have a consistent story about what these options are: they are defined in a single place, and you can provide some of the options as client defaults, and you can also provide any of the options per query.

Embedding the query options breaks the existing API. It makes client configuration (when you need to set query options) a little more verbose, but I think makes it more clear what you are doing.

Also, this separation also makes type enforcement work better. Before, when client figuration was essentially ClientOptions & QueryOptions but there are some places (like Client.#setHeaders) that you only want the query options.

JSON types

Also noted that JSONObject and JSONValue are necessary because we are not handling encoding/decoding of user classes.

ptpaterson avatar Mar 03 '23 06:03 ptpaterson

@ptpaterson IMO this makes things a bit confusing.

The QueryRequestHeaders type showed users what the valid headers were.

And the wire protocol allows for every possible header to also be sent in the request body which was the reason for the original implementation.

QueryRequestOptions just seems like an oblique name for headers.

Headers themselves are also part of the wire protocol which is why they were in that file.

Perhaps we could just make it more clear that any header can be sent in the request body with documentation.

The fact it "extends" the headers is just a way to implement it without repeating ourselves.

doom-weaver avatar Mar 03 '23 06:03 doom-weaver

That makes sense, but then I think the spec must have changed before I jumped on board.

I'll need to try this out. And also dig into the spec history to see when this changed, and be really sure it has changed.

ptpaterson avatar Mar 03 '23 11:03 ptpaterson