Skip to content
This repository has been archived by the owner on Feb 9, 2020. It is now read-only.

Implement promise interface for non-stream-based commands #34

Closed
wants to merge 7 commits into from

Conversation

sam3d
Copy link
Contributor

@sam3d sam3d commented Oct 26, 2017

This PR implements a simple optional promise interface as outlined in #33. Luckily based on what you guys said, it doesn't break backwards-compatability (🎉) as these methods didn't return anything anyway. You can now use the non-streaming commands in the following ways:

Standard

serf.members((err, members) => {
    console.log(members);
});

With promises method

serf.members()
.then(members => {
    console.log(members);
})
.catch(err => {
    throw err;
});

With await/async

let members = await serf.members();

I've also included some brief documentation in the README, but I've had to include b6dd867 because I couldn't get it to work on my machine without including it. Once the other #31 has been merged I'll rebase this with master and remove that commit so it doesn't clog the log, so to speak.

@sam3d
Copy link
Contributor Author

sam3d commented Oct 26, 2017

Ah my workflow now looks like this:

let port = 5918;

let agent = await serf.agent(port);
let client = await serf.client(port);
let members = await client.members();

console.log(members);

And I can't tell you how happy that makes me that it looks this simple 😄

Copy link
Collaborator

@zbjornson zbjornson left a comment

Choose a reason for hiding this comment

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

Code looks good 👍. Would be nice to add a test for one of the methods.

README.md Outdated
@@ -42,6 +42,33 @@ var client = Serf.connect(function (err) {
});
```

All of them non-stream-based commands also have a promise interface if you'd
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor typo: s/them/the

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops my bad, will fix that now and add a test 👍

zbjornson
zbjornson previously approved these changes Oct 26, 2017
@zbjornson zbjornson requested a review from hden October 26, 2017 21:10
@sam3d
Copy link
Contributor Author

sam3d commented Oct 26, 2017

Wait no the tests are failing, I'll give that a look-over bear with me.

@zbjornson
Copy link
Collaborator

Hm, actually I wonder if it should only return a Promise if there is no callback provided. It's unfortunate to get UnhandledPromiseRejectionWarnings if you're not using promises -- especially since you're probably handling the err argument in the callback.

@sam3d
Copy link
Contributor Author

sam3d commented Oct 26, 2017

Good plan, let's give that one a try.

@sam3d
Copy link
Contributor Author

sam3d commented Oct 26, 2017

Okay so this is odd - the particular test case now passes but it breaks all future test cases:

Serf stream works with a "listen" callback:
 Uncaught Error: This socket has been ended by the other party
      at Serf.writeAfterFIN [as write] (net.js:290:12)
      at dowrite (serf.js:204:10)
      at Serf.send (serf.js:188:5)

@sam3d
Copy link
Contributor Author

sam3d commented Oct 26, 2017

Okay after testing locally this happens too, I hadn't tested multiple access types. For some reason when attempting to make another request after the first promise-based one, none of the other requests work. Will look into this now.

@zbjornson
Copy link
Collaborator

You can monitor the serf process's output with something like

+    procs[0].stdout.on('data', d => console.log(d.toString()))
+    procs[1].stdout.on('data', d => console.log(d.toString()))
+    procs[0].stderr.on('data', d => console.log(d.toString()))
+    procs[1].stderr.on('data', d => console.log(d.toString()))

and get...

2017/10/26 21:47:11 [ERR] agent.ipc: Failed to evaluate request: command '' not recognized

I'm not seeing an obvious cause, but let me know if you need help debugging and I'll look more closely.

@zbjornson zbjornson dismissed their stale review October 26, 2017 21:49

Clicked too soon

@sam3d
Copy link
Contributor Author

sam3d commented Oct 26, 2017

Okay this could take some work. I'm gonna close this for now and revisit this specifically in perhaps a day or two; in the meantime I'm going to test out another solution that I've been working on that I hope to be able to implement into this.

@sam3d sam3d closed this Oct 26, 2017
@sam3d sam3d deleted the promise-interface branch October 26, 2017 23:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants