Added promises to the client-side socket.emit event.

[happilymarrieddad]

I added promise support to the client-side emit event. I could really use this for V4 of our app. It will allow us to integrate Async/Await with our app more easily without having to use a wrapper.

const res = await socket.emit('getData',{ id:1 }).catch(err => { throw new Error(err) })

is so much better than using a wrapper. Thanks!

If this gets approved, I am going to attempt to add promises throughout SC and hopefully at some point that it will be available all over the API. Thanks!

npm run test::promises

to test it.

[jondubois]

@happilymarrieddad Great. This is a good start and I agree that the time has come to bring promises to the socketcluster-client, but we should have a discussion about it before we merge any code into the master branch since this is going to be a major change. Here are some of my thoughts:

About this new feature:

1. We should aim to have full promise support for all relevant methods of the SC client before we can merge this feature into master and deploy it as a new release. Only allowing developers to use promises for some methods (like emit) and not others (like publish, subscribe...) is going to create confusion and frustration. At this stage of the project, we need to avoid inconsistencies as much as possible.
2. I think that promises are an all-or-nothing feature; either you use them or you don't - It's kind of unusual (and probably not a good idea) if a developer uses both the callback AND promise.then(...) style in different parts of their code. If we want to support promises, I think that we shouldn't support the old callback style at the same time.

Because of the two points above, we should consider either:

• Creating a new es6 release branch for the ES6/Promise client inside this socketcluster-client repo - Then we can deploy this new ES6 client to npm alongside the current client under a different package name (for a while at least).
• Creating a new repo (forked from the current socketcluster-client) and maintain the two projects separately (we can still merge relevant changes back and forth between the two repos - This has more friction than just creating a branch but it might help with contribution).

About your code:

• I've recently added a proper mocha test suite to socketcluster-client (see test/ directory), if you get the latest version of the repo, you can run it with npm test - We're moving away from ad-hock testing so we should adapt your test logic to the mocha format and start using describe, it and assert.
• Part of the reason why I think we should branch/fork a new client instead of changing the current one directly is because logic like Promise.toString().indexOf("[native code]") tends to be problematic; I think it's better to avoid this kind of feature detection if possible because it's rarely 100% reliable (when doing feature detection for socketcluster-client, I often find myself fixing issues with React Native, Cordova, IE, Android WebView, Opera browser, ... or some other weird JavaScript engine); so I think it's better to let the developer use different clients for different environments and not do any automatic feature detection (currently the client only does feature detection for native WebSockets vs ws but it's been a major pain). Also we don't want to keep adding if (self.allowPromises) ... in every single function.
• Regarding code style, we should try to follow the Crockford Style exactly (http://javascript.crockford.com/code.html); especially the part about function declarations and whitespace; being consistent with the code style helps when doing text search to find specific function declarations (I use sift on the command line)... Or when doing automatic search and replace.
• (Minor thing) You should try to rename your commits (or squashing them) before submitting a PR. Some developers like to look through the commit history to help them do client upgrades.

--

Let me know what you think about branching vs forking the current code to make the new ES6 client then we can proceed to the next stepa and merge some of your code (after modifications have been made).

[happilymarrieddad]

@jondubois Thanks so much for providing all that information. I'm going to close this pull request and attempt to recreate a full promise version of sc-client from scratch. Hopefully, I can get something together after a little while. Thanks again!

[jondubois]

@happilymarrieddad I created a new repo (forked from the current one) so you can do your PRs against that one instead https://github.com/socketcluster/socketcluster-es6-client - The main difference is that when you add promise support for a method, you should also remove callback support from that method. Also since this client/repo isn't live yet, you can do multiple small PRs against master branch (you can convert just emit function to promises - Unlike socketcluster-client, we don't have to keep it in a clean development state at this point).

Small PRs are actually better; that way we can discuss as we go.