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

Consider using Promise#nodeify in JS drivers #2973

Closed
dminkovsky opened this issue Aug 27, 2014 · 11 comments
Closed

Consider using Promise#nodeify in JS drivers #2973

dminkovsky opened this issue Aug 27, 2014 · 11 comments
Assignees
Milestone

Comments

@dminkovsky
Copy link

From IRC:

01:08 < phuh> neumino: Did you know bluebird has .nodeify() that helps simply code like this one?
https://github.com/rethinkdb/rethinkdb/blob/next/drivers/javascript/ast.coffee#L99-L126
01:09 < phuh> documentation says "There is no effect on peformance if the user doesn't actually pass a node-style callback function."

I've also been thinking about this for a while. I believe nodeify() would simply things in that span, and similar spans of the JS driver. nodeify() is cool and also provides an easy way to support the callback interface while using Promise.using() for a possible connection pool implementation (#281 (comment)).

@coffeemug
Copy link
Contributor

/cc @neumino. What do you think?

@coffeemug coffeemug added this to the subsequent milestone Aug 27, 2014
@neumino
Copy link
Member

neumino commented Aug 28, 2014

I actually used nodeify in rethinkdbdash for 1.14, and was planning to do the same with the official driver.

@neumino neumino self-assigned this Aug 28, 2014
@thelinuxlich
Copy link

+1

@neumino
Copy link
Member

neumino commented Aug 28, 2014

Part of this issue will be to bump blubird to 2.x

@dminkovsky
Copy link
Author

Totally did not notice we weren't on 2.x :)

neumino pushed a commit that referenced this issue Sep 27, 2014
@neumino
Copy link
Member

neumino commented Sep 28, 2014

In branch michel_2973_nodeify
Review 2149 assigned to @AtnNn and @Tryneus

The bluebird dependency is bumped to 2.3.2

@pilwon
Copy link

pilwon commented Sep 28, 2014

@neumino Also consider dropping the use of new Promise.

From Bluebird API doc:

Note: In Node.JS it is **very unlikely* that you will ever need to create promises yourself, see promisification*

Also refer to the deferred anti-pattern.

Suggestions:

  • .promisify()
  • Promise.method()
  • return Promise.resolve()
  • return Promise.reject()
  • re-throw error in .catch()

@neumino
Copy link
Member

neumino commented Sep 28, 2014

The idea behind Promise.promisifyAll is to get promises from libraries with asynchronous function (like fs), and then just using these promises.
The thing is that doesn't work for the net module.

For example, you could call promisify on socket.write but it will be resolved when the data is written out, not when you receive a response from the server (you actually have to parse the data sent form the server to know what query it matches first).

@pilwon
Copy link

pilwon commented Sep 29, 2014

@neumino I understand the use of new Promise is necessary for some cases (ex: cursor.coffee). However, couldn't we simplify this code with Promise.method or Promise.reject?

@neumino
Copy link
Member

neumino commented Sep 29, 2014

Oh, thanks @pilwon -- I didn't pay attention to the big try/catch there. I removed the try and just call Promise.reject.
Updated in branch michel_2973_nodeify.

I'm not a big fan of wrapping a method of a CoffeeScript class with Promise.method. I haven't tried it, but I wouldn't be surprised if it was to lead to tricky behaviors like this one #1948 (comment).

@neumino
Copy link
Member

neumino commented Oct 2, 2014

Merged in next as dbae9bd
Ship 1.16

@neumino neumino closed this as completed Oct 2, 2014
@AtnNn AtnNn modified the milestones: 1.16, subsequent Oct 2, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants