Initial support for email query and get actions #5

jeserkin · 2020-12-10T08:35:04Z

No description provided.

alagane

Overall it looks good to me, I don't know yet when it will merged.

src/types.ts

src/index.ts

chibenwa

Thanks a lot for this very nice contribution! Those remarks are not related to this (nice!) PR but more to this repository code base in general.

Simple question but it would be really nice to use back-reference to chain JMAP calls and reduce server roundtrips.

(For someone like me based in Vietnam this is actually critical!)

You could:

start with a mailbox query to retrieve the guy INBOX
Then back-reference the INBOX in the email/query
And finally back-reference the ids in the Email/get

Doing all of that with a single network roundtrip, with a single API call.

Is there planned support for back-references in jmap-client-ts?

I would advise the maintainer to implement it early, in order to avoid generating technical debt.

(No tests?)

I would suggest using linagora/james-memory docker image in order to start a JMAP server against which to test the client... :-)

Edit: I opened dedicated issues ;-)

#6
#7

chibenwa · 2020-12-11T02:09:48Z

.gitignore

+.c9/
+*.launch
+.settings/
+*.sublime-workspace


You can use a local, global ~/.gitignore for your own tool related stuff, a good practice is to put only build related ignores in a given project.

Sure. I can remove those.
Just as a separate question:

Wouldn't that make life easier for other developers who would join development contributions in the future? So that they would not need to discover, that "Ah, my IDE sees some workspace related files, that I don't want to accidentally add to commit"

Well, I don't mind if there are some lines in the .gitignore since there is a comment to explain what they are related to.

chibenwa · 2020-12-11T02:10:43Z

src/index.ts

-    list: IMailbox[];
-    notFound: string;
-  }> {
+  ): Promise<IMailboxGetResponse> {


👍 for this strong typing! <3

* Also added type for Email keywords

jeserkin · 2020-12-14T09:02:24Z

src/types.ts

  createdModSeq: number;
  updatedModSeq: number;
  deleted: Date | null;
+  receivedAt: Date;


I wonder if this and other Date fields should even be present as Date fields. You can't pass Date via JSON, it will be number or string, which end user will then need to convert. They look nice in type, but I feel like this is the wrong type for it. Library is intended to work with raw data. And raw data type for Date is string/number.

@alagane , @chibenwa thoughts?

Yes (here in the specs https://jmap.io/spec-core.html#the-date-and-utcdate-data-types)

We could use string or custom types

export type IDate = string; export type IUtcDate = string;

I think this ^ would work.

JMAP mandat String ( date-time format [@!RFC3339]): https://jmap.io/spec-core.html#the-date-and-utcdate-data-types

* Added type for boolean true value * Added UtcDate meta type as string * Removed date and deleted properties, since was unable to locate them in specs

alagane · 2020-12-15T12:42:12Z

deleted property was in https://jmap.io/server.html#3-mailboxes but this page is not up to date (jmap draft) so some properties may be wrong.

chibenwa · 2020-12-16T01:34:22Z

src/types.ts

+  $forwarded?: ITrue;
+  $phishing?: ITrue;
+  $junk?: ITrue;
+  $notjunk?: ITrue;


I'm not fluent in JavaScript but I got the feeling we only offer a close set of Keyworks.

JMAP allows passing arbitrary keywords. Set<String> is likely a better data structure to represent it (convenience methods can be exposed)

At least it can come in follow up refactorings.

Those keywords are related only to Email entity https://jmap.io/spec-mail.html#metadata as far as I can tell.

However:

Users may add arbitrary keywords to an Email. For compatibility with IMAP, a keyword is a case-insensitive string of 1–255 characters in the ASCII subset %x21–%x7e (excludes control chars and space), and it MUST NOT include any of these characters

I would assume, that it could be added in future changes. Why I am offering it at the moment? Due to it still being in early stages it is easier to limit our logic to use of interfaces, enums and types. If we start to introduce classes it might become a bit more complex.

Due to JSON structure being returned the way it is (regarding keywords), we can't set it as Set without some conversion logic.

chibenwa · 2020-12-16T01:36:21Z

src/types.ts

  attachments: Attachment[] | null;
  createdModSeq: number;
  updatedModSeq: number;
-  deleted: Date | null;


What are these createdModSeq updatedModSeq properties?

Looking at it in the spec I have 0 matches...

Is it necessary?

(I agree, this is not related to your work)

I was wondering the same, but decided to lessen the overhead of changes. I was unable to locate those as well. However, they are present here https://jmap.io/server.html#3-mailboxes and as far as I understand, it is (draft aka not based on confirmed standard). Maybe @alagane can add some more detail in regards to it?

jeserkin · 2020-12-17T04:07:09Z

deleted property was in https://jmap.io/server.html#3-mailboxes but this page is not up to date (jmap draft) so some properties may be wrong.

At the moment when I make Postman requests towards James I don't have this property in response. This is why I removed it at the moment.

Also as a separate question. If I plan to add more changes, can I base them on this PR? Don't really want to introduce anymore changes here if they are not related to original changes, that I made.

alagane · 2020-12-17T07:50:50Z

I merged it, you can also create issues if needed, for example for things that are unrelated to the merge request but need to be done later.
In issues we can also discuss about some point that we would not agree on.

Initial support for email query and get actions

53ba313

alagane requested changes Dec 10, 2020

View reviewed changes

src/types.ts Outdated Show resolved Hide resolved

src/types.ts Outdated Show resolved Hide resolved

src/index.ts Outdated Show resolved Hide resolved

Changes based on PR linagora#5 feedback

061a81b

jeserkin requested a review from alagane December 10, 2020 22:15

chibenwa reviewed Dec 11, 2020

View reviewed changes

alagane approved these changes Dec 11, 2020

View reviewed changes

Added missing receivedAt email property

cc10f61

* Also added type for Email keywords

jeserkin commented Dec 14, 2020

View reviewed changes

alagane approved these changes Dec 14, 2020

View reviewed changes

Added missing receivedAt email property

19453cb

* Added type for boolean true value * Added UtcDate meta type as string * Removed date and deleted properties, since was unable to locate them in specs

alagane requested a review from chibenwa December 15, 2020 16:50

chibenwa reviewed Dec 16, 2020

View reviewed changes

alagane merged commit a0eb950 into linagora:main Dec 17, 2020

Initial support for email query and get actions #5

Initial support for email query and get actions #5

Uh oh!

Conversation

jeserkin commented Dec 10, 2020

Uh oh!

alagane left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chibenwa left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alagane Dec 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeserkin Dec 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alagane Dec 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alagane commented Dec 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeserkin commented Dec 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alagane commented Dec 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chibenwa left a comment •

edited

Loading

alagane Dec 11, 2020 •

edited

Loading

jeserkin Dec 14, 2020 •

edited

Loading

alagane Dec 14, 2020 •

edited

Loading

alagane commented Dec 15, 2020 •

edited

Loading

jeserkin commented Dec 17, 2020 •

edited

Loading