Skip to content
This repository has been archived by the owner on Jan 30, 2024. It is now read-only.

Reduce client module duplication #81

Closed
niklaslong opened this issue Oct 13, 2020 · 4 comments · Fixed by #83
Closed

Reduce client module duplication #81

niklaslong opened this issue Oct 13, 2020 · 4 comments · Fixed by #83

Comments

@niklaslong
Copy link
Owner

As mentioned in #77, this issue was created to discuss the future architecture of the library.

From #77:

...which causes too much duplication of code, docs and tests, which are hardening development, since it consumes a lot of time.

Agreed, duplication has been one of the greatest pain points of the project so far and reducing that by stripping down the Client module is probably the way to go. In my mind, the Client has always been there to tie the non-io part of the library to the HTTP layer. I think a good first step might indeed be to move to this pattern:

Request.logout("http://base_url", token)
|> Client.do_request()

The downside which has stopped me from doing it thus far, is that two function calls would be necessary for each request. The Client module ends up being a thin wrapper for the configured HTTPClient which makes me wonder if it is even necessary. Thoughts?

There also could be situations, when request should not be executed at all. For example: In most of cases download, thumbnail methods are needed to generate some urls, but not to do actual download. [...] But right now there is no context module to place such methods (it could be MatrixSDK.Client, but currently its concern is quite blurred).

Good point, and yes, it is quite blurred. I'm not sure what the solution is here yet. My hunch is that the Client module should imply IO, and the Request module should not; this might be a helpful distinction to keep in mind to start reducing the coupling. This might also be cleared up once #78 is discussed?

@inaniyants
Copy link
Contributor

inaniyants commented Oct 13, 2020

The downside which has stopped me from doing it thus far, is that two function calls would be necessary for each request.

As for me it's normal for functional approach, where each function does well only one thing and developer need to compose this functions in a way, that is required.

But you are correct - it looks like quite low level. I need also to think more about possible abstractions, that could hide this low level operations from library enduser.

There could be opposite approach for eliminating duplication, but it also not ideal:
clear Request module from any builder functions and build all directly in Client module. In this way Request will define struct and some helper functions or types, but actual building will be applied in Client module. In this case the only problem will be a size of Client module. If we will decide to split it on multiple modules, then better naming is needed from start (similar to suggested in #78).

@niklaslong
Copy link
Owner Author

I'd be more in favour of the former and not the latter in this case as it would couple the IO with the Request struct building.
We could go with the two function approach: the Client module is a wrapper around IO, making the requests, parsing the errors/response (this should be implemented elsewhere of course—perhaps an http module needed)? The Client may also need to wrap encryption related things in future as well.

We can reevaluate at a later stage if this isn't the right abstraction but it's probably a good starting point?

@fancycade
Copy link
Collaborator

fancycade commented Oct 13, 2020

Request.logout("http://base_url", token)
|> Client.do_request()

Is nice from a functional standpoint and seems quite common with Elixir libraries. There are some nice advantage to this apporach.

  1. Allows user to test/validate the request before sending it.
  2. We can eliminate the client module mostly, and the tests associated with it, which makes it easier to contribute. When new endpoints are added.

I am afraid that at some point someone will just write a wrapper that is simply a big Client module, but I guess that is there choice.

@niklaslong
Copy link
Owner Author

Request.logout("http://base_url", token)
|> Client.do_request()

Let's go with this for now, at the very least it will facilitate the development of the library. In light of what was said in #79, I'm happy for the Client module to be repurposed as the bridge between the Request layer and the HTTP layer (with response parsing and error handling).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants