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

Change error handling to make it harder to end up with uncaught errors #16

Open
torkildr opened this issue Sep 5, 2018 · 1 comment
Labels
enhancement New feature or request task

Comments

@torkildr
Copy link
Contributor

torkildr commented Sep 5, 2018

Today, it is possible to end up with a case where, because of delayed execution, the error emitter is assigned after some of the error-emitting code has started to execute.

Example of delayed emitter assignment

const xapi = jsxapi.connect('ssh://localhost:99');
Promise.resolve(() => {
  xapi.on('error', (err) => {
    console.error(`xapi error: ${err}`);
  });
});

Though this example is a bit contrived, it is easy to picture a more organic scenario where the code is "synchronous" ish, but is triggered in an asynchronous manner, thus not being executed before any other async logic.

We should investigate the possibility of moving the error emitter assignment so that it is possible to do before doing the initial commit.

An example, though not the final design, of such a solution, might be something like:

import Jsxapi from 'jsxapi';

const jsxapi = new Jsxapi();

jsxapi.on('error', err => console.log(err));
jsxapi.connect('ssh://localhost:99')
@torkildr torkildr added enhancement New feature or request task labels Sep 5, 2018
@myme
Copy link
Contributor

myme commented Sep 16, 2018

I can see that this might cause issues, due to the delayed evaluation of the code adding an error handler. It's worth noting though, that although this might be confusing to users, I would suggest to firstly improve communication regarding error handling (through examples and documentation). This particular case is solvable in a quite trivial manner: ensure to add listeners synchronous (or perhaps more clearly, in the same "tick" as the .connect() invocation.

const xapi = jsxapi.connect('example.com');
xapi.on('error', console.error);

Since .on() returns the instance, it can even be chained:

const xapi = jsxapi.connect('example.com')
  .on('ready', () => { /* .... */ })
  .on('error', console.error);

To have this work in a Promise-ified context (in this case using async/await), one might use something like the following snippet:

async function connect(host, options) {
  return await new Promise((resolve, reject) => {
    const xapi = jsxapi.connect(host, options)
      .on('error', (err) => { reject(err); })
      .on('ready', () => { resolve(xapi); });
  });
}

As I would say this is something I consider quite idiomatic JavaScript/Node, I'm not sure I agree that additions to the API is the right course of action.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request task
Projects
None yet
Development

No branches or pull requests

2 participants