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

Separate low and high level API #46

Open
alagane opened this issue Mar 30, 2021 · 14 comments
Open

Separate low and high level API #46

alagane opened this issue Mar 30, 2021 · 14 comments

Comments

@alagane
Copy link
Member

alagane commented Mar 30, 2021

Why?

To easily know which methods are low and high level.

How?

Separate files?

// Low level
client.lowLevel.mailbox_get(...);

// High level
client.getAllMailboxes(...);

About mailbox_changes

Issue #45
What about the case of mailbox_changes to get changes until hasMoreChanges is false?
Is it low level because its response is the same, and it just mimics the simple mailbox_changes using the same data structure?
Or is it high level because it does more than just making a simple JMAP request?

@chibenwa
Copy link
Member

I would maybe put it the other way around?

// Low level
client.mailbox_get(...);

// High level
client.highLevel.getAllMailboxes(...);

@chibenwa
Copy link
Member

Or is it high level because it does more than just making a simple JMAP request?

We should make it clear that several calls could be made and highLevel is IMO a nice way to depict that.

@alagane
Copy link
Member Author

alagane commented Mar 30, 2021

I would maybe put it the other way around?

// Low level
client.mailbox_get(...);

// High level
client.highLevel.getAllMailboxes(...);

Well, it might be better to do it like this if:

  • We chose the low level API is the main one
  • Assume that some functions do not exist on high level API

I would do it the other way if:

  • We chose the high level API as the default one (no now for sure)
  • The high level is supposed to be simpler, so using it would not require to use .highLevel

We should make it clear that several calls could be made and highLevel is IMO a nice way to depict that.

Ok, so we could keep the structure or change it (for the high level one) to remove the unused property (hasMoreChanges) that would always be true.

@jeserkin
Copy link
Collaborator

Could you elaborate on benefits of that separation? Is it purely for OpenPaaS? Because at the moment I look at it and don't clearly understand what does it mean.

@chibenwa
Copy link
Member

chibenwa commented Mar 31, 2021

Hello @jeserkin,

This library so far maps directly JMAP calls (which could be seen as low level). However, there is a couple of higher level primitives that could be used to build a mail user agent, are generic, but might be slightly opinionated. #45 is one of them.

Also, rather than writing a generic back-reference mechanism (an API doing some kind of data validation would be hard to do), we are thinking about writing some higher level functions in the client. Like "list the mails in the INBOX", "mark all mails as read", "clear the trash" - common utilities that lib users can rely on, or not. Lower level primitives would still here if need be.

JMAP clients in the Android space tend to do the distinction (https://github.com/iNPUTmice/jmap jmap-client VS jmap-mua).

Hence the idea of low level VS high level client.

Does it helps?

Cheers,

Benoit

@Arsnael
Copy link
Member

Arsnael commented Mar 31, 2021

Same as @jeserkin , how do you define here what is high and low level? What is your context around it?

@jeserkin
Copy link
Collaborator

jeserkin commented Mar 31, 2021

@chibenwa I see. So basically if at the moment I have something like this

from(this.fetchSession())
  .pipe(
    mergeMap(session => from(this.jmapClient.mailbox_get({
      accountId: Object.keys(session.accounts)[0],
      ids: null
    })))
  );

then it would change into something more like this from(this.jmapClient.getAllMailboxes())

This is nice, but based on your discussion I would suggest following structure.

By default you offer Higher Level API as this.jmapClient.getAllMailboxes() and if developer wants to customize request and do lower level API calls, then it could be something like

const lowLevelClient = this.jmapClient.getLowLevelClient(); // This will either create or if lowLevel API is already used under the hood, then just return it
from(this.fetchSession())
  .pipe(
    mergeMap(session => from(lowLevelClient.mailbox_get({
      accountId: Object.keys(session.accounts)[0],
      ids: null
    })))
  );

@chibenwa
Copy link
Member

I agree with @jeserkin remark, that is fine for me.

(the most important thing IMO is to have a clear separation beteen both)

@jeserkin
Copy link
Collaborator

jeserkin commented Mar 31, 2021

Mhm. I would also like to have single method for updating draft.
Currently I am doing 2 calls to JMAP Client. Email/set for create and Email/set for destroy. Would be nice to have higher level API call for updateDraft (but that will require low level API development to support back reference calls).
The only issue in my case with current implementation of updating draft is that I can't provide accurate timestamp, when it was initially created, since I am constantly destroying and creating it over and over again, but that is a separate case.

@jeserkin
Copy link
Collaborator

jeserkin commented Apr 5, 2021

I would like to add methods for uploading and downloading attachments, but I am struggling with where would they belong? Whether lower level or higher level API? Thoughts? @chibenwa, @alagane

@chibenwa
Copy link
Member

chibenwa commented Apr 5, 2021

(but that will require low level API development to support back reference calls)

Why do you need back references for updating a draft? You want to cary over information from the old draft to the new without specifying it in the request?

To be honest, I think getting back references working on a typed manner on top of the lowLevel clients will be really challenging.

I would support the idea that high level calls backed by backreferences are typed themselves, but could rely on raw JSON over the transport layer. An alternative to this might be to duplicate the properties the highLevel clients needs backreference support on, in order to have the #property version of property...

The only issue in my case with current implementation of updating draft is that I can't provide accurate timestamp, when it was initially created, since I am constantly destroying and creating it over and over again, but that is a separate case.

Ok this is your use case!

Doing 2 API calls would solve it, at the price of more roundtrips...

@chibenwa
Copy link
Member

chibenwa commented Apr 5, 2021

I would like to add methods for uploading and downloading attachments, but I am struggling with where would they belong? Whether lower level or higher level API? Thoughts? @chibenwa, @alagane

IMO it is clearly low level :-)

@alagane
Copy link
Member Author

alagane commented Apr 6, 2021

I would like to add methods for uploading and downloading attachments, but I am struggling with where would they belong? Whether lower level or higher level API? Thoughts? @chibenwa, @alagane

Can you create a new issue? I do not really see what you want to do with these methods, so I do not know if it is in the scope of the project, so we can discuss about it.
If it will be high or low level can be defined/changed after a pull request is created if needed.

@jeserkin
Copy link
Collaborator

jeserkin commented Apr 6, 2021

Sure. No problem.

UPD:
@alagane here you go #49

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

No branches or pull requests

4 participants