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

Use node-style callbacks throughout #38

Closed
maxkueng opened this issue Apr 30, 2014 · 6 comments · Fixed by #43
Closed

Use node-style callbacks throughout #38

maxkueng opened this issue Apr 30, 2014 · 6 comments · Fixed by #43
Assignees
Milestone

Comments

@maxkueng
Copy link
Contributor

Callbacks should be passed an Error as the first argument and data as the second.

Like so:

client.logIn(function (err, data) {
  if (err) { return console.log('login failed', err); }

  console.log('login successful', data);
});

Instead of:

client.logIn(function (data) {
  console.log('maybe login successful', data);
});
@maxkueng maxkueng changed the title Use Node-style callbacks throughout Use node-style callbacks throughout Apr 30, 2014
@mitar
Copy link

mitar commented Jul 20, 2014

+1

@mitar
Copy link

mitar commented Jul 20, 2014

So, yes, how does one detect errors at all?

@mitar
Copy link

mitar commented Jul 20, 2014

Or, you could expose promises and allow user to hook into success and failure callbacks.

BTW, you also throw exceptions in the code in some callbacks, this is not a good pattern because it can kill the whole server.

@Fannon
Copy link
Contributor

Fannon commented Oct 18, 2014

+1

If an error happens, this library does kill my app regularly.

@macbre macbre self-assigned this Oct 21, 2014
@macbre macbre added this to the v0.4 milestone Oct 26, 2014
@MattiSG
Copy link

MattiSG commented Jan 5, 2015

+1

Considering using nodemw in production, but this is a dealbreaker. Any timeline? :)

@macbre
Copy link
Owner

macbre commented Jan 10, 2015

Pull request (#43) issued with proposed fix. I'm also considering removing the support for promises from nodemw.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants