Skip to content

[WIP] Promisify client-facing methods #225

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

Closed
wants to merge 1 commit into from
Closed

[WIP] Promisify client-facing methods #225

wants to merge 1 commit into from

Conversation

alecgibson
Copy link
Collaborator

@alecgibson alecgibson commented Jul 19, 2018

This change makes client-facing methods return a Promise when no callback is provided.

Motivation

  • As a consumer of ShareDB using modern JavaScript with Promises and async/await, I'm bored of using callbacks, which are inconsistent with the rest of my codebase
  • As a contributor to ShareDB, I'm bored of tests with deeply nested callbacks (and other maintenance burdens imposed by callbacks)

Work so far

This is a work-in-progress pull request in order to get opinions on how people feel about this as a feature; and how it should be implemented.

So far, I have added a promisify method to util, which has the following signature:

util.promisify = function(callback): { promise, callback }

If a callback is provided, then promise is undefined, and the returned callback is the same as the provided callback. This is to avoid clients inadvertently creating logic forks, where code follows both a callback and a Promise.

If callback is not a function, then the returned callback is a function that will resolve or reject the returned promise.

Also, for exiting functions early, callback() will return the promise, so you can write things like: if (condition) return callback();.

Discussion

Through this work I've discovered at least two points for discussion:

  • Implementing this in Doc as it currently is means that emit is basically never called, because callback will now always be defined. I don't know enough about how people use these events, but we could potentially have consumers configure ShareDB to enable promises? If promises are not enabled, then util.promisify will always return the same callback as provided (even if it's falsy).
  • The signature for a Query callback is callback(error, results, extra), but a Promise can only resolve a single argument. Should we combine this into { results, extra } for the purposes of a Promise?
  • Do we bother including a polyfill for Promise, or just not bother promisifying if Promise is undefined? (And if we avoid a polyfill, how do I get the linter to calm down without targeting ES6?)

This change makes client-facing `Doc` methods return a `Promise` when
no `callback` is provided.

In order to do this, a `promisify` method is added to `util`, which will
return a `{ promise, callback }` object. Calling `callback()` will also
return the `promise` so that early `function` exits are possible with
`return callback();`
@coveralls
Copy link

coveralls commented Jul 19, 2018

Coverage Status

Coverage decreased (-82.2%) to 14.336% when pulling 2b1f966 on alecgibson:promises into 762e05d on share:master.

@alecgibson alecgibson mentioned this pull request Jul 19, 2018
@alecgibson
Copy link
Collaborator Author

Closing this, as apparently @nateps is not keen on the idea of Promises in this library.

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.

2 participants