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

Add promises #81

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add promises #81

wants to merge 1 commit into from

Conversation

sualko
Copy link
Contributor

@sualko sualko commented Sep 19, 2017

I did just some quick tests, but I think it needs some more exhaustive ones.

@arlolra
Copy link
Owner

arlolra commented Sep 19, 2017

First things first, looks like we need to get the test suite running on Travis again.

@arlolra
Copy link
Owner

arlolra commented Oct 10, 2017

The grunt fix here was merged as 25bfc26.

Copy link
Owner

@arlolra arlolra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, you probably want to reject promises in the those functions, rather than call notify.

Also, this could use a rebase.

@@ -572,7 +574,7 @@
if (this.REQUIRE_ENCRYPTION) {
this.storedMgs.push({msg: msg, meta: meta})
this.sendQueryMsg()
return
return promise
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This promise is never rejected or fulfilled, similarly all the cases below where the functions return early.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was by purpose, to resolve a Promise only if there were some data to send. But yes in this case sendQueryMsg should return a promise and also notify should return an rejected promise. Will update the pr accordingly.

@@ -583,45 +585,53 @@
case CONST.MSGSTATE_FINISHED:
this.storedMgs.push({msg: msg, meta: meta})
this.notify('Message cannot be sent at this time.', 'warn')
return
return promise
case CONST.MSGSTATE_ENCRYPTED:
msg = this.prepareMsg(msg)
break
default:
throw new Error('Unknown message state.')
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably want to reject here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Throwing an exception is the same as rejecting.


switch (msg.cls) {
case 'error':
this.notify(msg.msg)
return
return promise
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return promise.reject(msg.msg), no?

@sualko
Copy link
Contributor Author

sualko commented Oct 11, 2017

I just encountered that I have to add promises to a lot more functions, like doAKE and so on. This will take a lot more time and I have to think about a smart polyfill solution.

@arlolra
Copy link
Owner

arlolra commented Oct 11, 2017

Ok, there may be some hints in #75 which promisifies some portion of the code.

I did just some quick tests, but I think it needs some more exhaustive ones.

Yes, adding to the test suite would be beneficial.

@jcbrand
Copy link
Contributor

jcbrand commented Oct 16, 2017

@sualko Concerning a polyfill, you can try es6-promise.

@sualko
Copy link
Contributor Author

sualko commented Oct 18, 2017

Before I continue, we have to decide when receiveMsg, sendMsg and endOtr should resolve. In my opinion promises should not replace the io handler, because e.g. during the AKE there are multiple messages exchanged and a promise can resolved only once. We also have to decide if a notification (this.notify) is the same as an error which leads to rejection. For me it's not and therefore I believe we should not resolve or reject the promise.

The aim of promises in this case (at least for me) is to get a relation between a plaintext and encrypted message, which is ensured in the current pr.

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

Successfully merging this pull request may close these issues.

3 participants