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

optional callback #55

Merged
merged 1 commit into from
Dec 11, 2014
Merged

optional callback #55

merged 1 commit into from
Dec 11, 2014

Conversation

reqshark
Copy link
Contributor

there are occasions when i dont care about server responses to xhr()

when the server response doesn't matter but you still want to fire-and-forget some type of request, then the callback ought to be optional:

xhr({
  body:JSON.stringify(whatever),
  uri:'/foo',
  headers:{'content-type':'application/json'}
})

this makes the callback more optional by preventing an error when
once() calls apply() on the unregistered (meaning non-existent)
callback, i.e. not passed to the exported xhr() constructor.

this makes the callback more optional by preventing an error when
`once()` calls `apply()` on the unregistered (meaning non-existent)
callback, i.e. not passed to the exported xhr() constructor
@naugtur
Copy link
Owner

naugtur commented Dec 11, 2014

Nice!
Thanks for catching that.

naugtur added a commit that referenced this pull request Dec 11, 2014
@naugtur naugtur merged commit a3cfae7 into naugtur:master Dec 11, 2014
@Raynos
Copy link
Collaborator

Raynos commented Dec 12, 2014

No. callbacks are not optional, when doing IO errors have to be handled.

We should throw an exception when you do not pass a callback.

Edgecase: when opting into sync IO the callback is optional.

@reqshark
Copy link
Contributor Author

Yep I had a feeling you would not want optional callbacks. And I tend to agree.

However, the callback here remains a requirement, though it may not be explicit for users when the library's noop satisfies the required callback function placement as a template.

Before you revert, consider the example set by the levelup API. When we put a new record into a leveldown backend the err callback is not required. Yet this amazing storage API handles the write.

That is why ultimately this is a trust issue. As a user I trust the levelup API just as I trust xhr in the browser.

User belief in the API is enough to handle the need for extra characters written as the callback. And beyond that trust, Node just basically garbage collects whatever that entails.

This is a win for convenience and its use case is the need to fire and forget.

@Raynos
Copy link
Collaborator

Raynos commented Dec 12, 2014

@reqshark

Before you revert, consider the example set by the levelup API. When we put a new record into a leveldown backend the err callback is not required. Yet this amazing storage API handles the write.

https://github.com/rvagg/node-levelup/blob/master/lib/util.js#L75-L79

function dispatchError (levelup, error, callback) {
  return typeof callback == 'function'
    ? callback(error)
    : levelup.emit('error', error)
}

Having an optional callback and falling back to emitting an error is fine. Having an optional callback and falling back to a noop is terrible.

Since there is no event emitter to emit an error on we have no choice but to make the callback required.

I would be slightly more open to xhr.fireAndForget(opts). i.e. a seperately named method that takes no callback.

Changing the interface in a way where its possible for a developer to have a bug of forgetting the callback is not acceptable.

@reqshark
Copy link
Contributor Author

I can't argue that type of logic. I'll update this with a FireAndForget(opts) method PR for your review before lunchtime tomorrow

@reqshark
Copy link
Contributor Author

@Raynos what do you think about a { fireAndForget: true } option?

if set to true, that could invoke a callback optional mode.

this way we don't change the interface and have to be deliberate about not passing the callback

@reqshark
Copy link
Contributor Author

#57

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