Split Request module into multiple Request builder modules

[inaniyants]

First of all I want to mention, that all coming next is just a suggestion, which could be modified or declined via discussion.

Right now Request module consists of 2500+ lines of code and docs. And this number will increase greatly in future, since in current state library does not support many existing client-server matrix endpoints. But there are much more endpoints, except client-server https://matrix.org/docs/spec/#id6

don't forget about administrative methods, which are not documented on matrix.org: https://github.com/matrix-org/synapse/tree/master/docs/admin_api

So, it is better to keep MatrixSDK.Client.Request module clear and consist only from struct definition, common types definition , and some context methods, which are helping to build request (remove\add header, etc...).

In order to build Request, I suggest to implement multiple modules (each for it's own concern), like: ClientRequest, AdminRequest, etc.

ClientRequest could be divided even more, like: ContentRequest, AccountRequest, RoomRequest, etc....

Modules structure could be, like:

request
  |_ request.ex
  |_ client
     |_ content_request.ex
     |_ account_request.ex
     |_ room_request.ex
     |_ ...
  |_ admin
     |_ ...

What do you think about this suggestion?

[niklaslong]

Great suggestion, this is something I've thought about quite a bit in the past and this is definitely necessary given the size of the Request module.

I'm not convinced by Client.Request.Client however or generally having duplication in the module names as it doesn't add clarity. I'll have a think to see if I can come up with clearer naming.

[niklaslong]

@inaniyants we could rename as follows:

Client -> API

And then go with a tree like this:

matrix_sdk
├── api
│   ├── request
│   │   ├── admin
│   │   │   └── rooms.ex
│   │   ├── admin.ex
│   │   ├── client
│   │   │   ├── account.ex
│   │   │   └── room.ex
│   │   └── client.ex
│   └── request.ex
├── api.ex

This would also be more consistent with what is being done in the Rust equivalent of this project. Thoughts?

[inaniyants]

@niklaslong such renaming and tree looks good. This will greatly simplify life for library contributors. Let's discuss few things on how to simplify life for library endusers after this change.

Example1:

alias MatrixSDK.Api
alias MatrixSDK.Request.Client.Account

# Account module directly used for request constructing
def call_some_matrix_endpoint() do
  Account.logout()
  |> Api.do_request()
end 

Example2:

alias MatrixSDK.Api
alias MatrixSDK.Request

# Aliasing Request only, calling to it's child namespaces
def call_some_matrix_endpoint() do
  Request.Client.Account.logout()
  |> Api.do_request()
end

In the example above there are 2 problems:

1. Request modules have confusing aliases by default. That's why I suggested to suffix request modules with Request, for example MatrixSDK.Request.Client.AccountRequest. But I agree that this looks too excessive.
2. user needs to know which request module he needs to alias (import) for some specific need. It's ok for low level library usage, but we should also made some default approach, which then we will reference in all examples.

So, we could:

• Neither import all child module functions into root context

Example3:

alias MatrixSDK.Api
alias MatrixSDK.Request.Client

# Client module imports Account module functions
def call_some_matrix_endpoint() do
  Client.logout()
  |> Api.do_request()
end 

• OR leave this code decomposition behind the scenes and import all child request functions into MatrixSDK.Request, then library api will be the same as before.

Example4:

alias MatrixSDK.Api
alias MatrixSDK.Request


# Import all functions into Request module
def call_some_matrix_endpoint() do
  Request.logout()
  |> Api.do_request()
end 

• OR we could define __using__ in Api, which could alias request modules with more meaningful names (still need to work more on naming)

Example5:

# Which aliases MatrixSDK.Request.Client as ClientRequest and aliases MatrixSDK.Api as MatrixApi
use MatrixSDK.Api, :client

def call_some_matrix_endpoint() do
  ClientRequest.logout()
  |> MatrixApi.do_request()
end 

[niklaslong]

Thanks for listing all the options!

Point taken on encouraging a convention and I agree this is important going forward. I think these will emerge fairly naturally as this split is undertaken and as more elements make their way into the library (response parsing, encryption, ...). The Elixir API guidelines also suggest preferring alias to use or import in this case. I'd be inclined to aim for convention around how to alias, rather than providing a use or an import.

[kowsheek]

I'm new to this library so I'm missing lots of context so feel free to correct me. I'm planning to use this for the use-case I mentioned in #90.

My initial thoughts are that the Rust SDK isn't very well done. We should reference more the JS SDK. The modules here roughly match the spec and is easier to follow.

Another reference SDK can be aws-elixir.

With these thoughts in mind, I propose the following structure:

matrix_sdk
|_client
  |_request
  |_service_discovery
  |_authentication
  |_events
  |_rooms
  ...

Where the request module then would be only responsible for sending & parsing HTTP requests. Looking forward to your thoughts.

Thanks again for the work on this!

[niklaslong]

@kowsheek Agreed, and thank you for the references. I think a flatter structure would be beneficial. Generally all the trees thus far have looked fairly similar (minus some fairly superficial nesting), I think the main point is to undertake a split as the request module is unwieldy.

Another aspect relevant to this split I'd like to open for discussion is the how the requests are being built. I think the request module could probably just have a request.new or request.from with the values corresponding to the struct keys being passed in. The rest of the modules could then just compute the arguments when needed and call the aforementioned.

Where the request module then would be only responsible for sending & parsing HTTP requests.

I initially wrote the SDK with a deliberate seperation between the request building and the HTTP layer, I think this distinction should probably remain?

[kowsheek]

I initially wrote the SDK with a deliberate seperation between the request building and the HTTP layer, I think this distinction should probably remain?

Yes, agreed!

I'll work on a PR :)