Node: Promisify of method without last argument callback - undefined beahvior

[benjamingr]

Currently, our documentation states:

promisify() assumes that original is a function taking a callback as its final argument in all cases, and the returned function will result in undefined behavior if it does not.

However, undefined behavior is a very "harsh" way to put what happens (since it implies the process can crash) and the behavior is pretty well defined.

• If a non function is passed in - promisify throws. https://github.com/nodejs/node/blob/master/lib/internal/util.js#L257-L259
• If a function is passed but its promisify.custom is specified but not a function - it throws.
• If a function is passed, but its last argument is not a node style callback - it will treat it 'as if' its last argument is a node style callback - and will pass one to it as the last argument.

It won't do something _useful_ but the behavior is defined.

I think our documentation should be amended to explain that the undefined behavior here isn't "as undefined" as writing out of bounds of a buffer for example.

I'm marking this as "good first issue", and "mentor available" in case someone wants to PR this but isn't sure how and would like guidance.

[mithunsasidharan]

@benjamingr : I'd like to take this up.

[Trott]

@benjamingr : I'd like to take this up.

@mithunsasidharan I would request that people who already have multiple commits into the project please not take good first issue tasks that have been open for only an hour. If it's been open for a week, then fine. But please leave a reasonable amount of time for these to be picked up by actual first-time contributors. Please don't treat good first issue as the easy pile. It takes away a good first contribution from someone who could use it.

[Trott]

By the way, to be clear: Totally go for stuff labeled help wanted or mentor available, but the good first issue stuff should be left alone unless it's been open for a while. Let actual first-time contributors have an opportunity at those.

If it helps, Here's a list of suggested ways to find good issues/tasks if you've already contributed but aren't sure what to work on next: http://nodetodo.org/next-steps/

[apapirovski]

As you said originally, I think once an issue has been around for longer than a week then it should be open to anyone. Based on reviewing a lot of the good first issues, the difficulty level varies greatly which I think explains why some are getting so little attention.

[mithunsasidharan]

@apaprocki @Trott : I've closed the PR. I'll leave it some new users as you've pointed out. I'm working on something else. Thanks !

[benjamingr]

@mithunsasidharan thanks and thanks for understanding - if you'd like ideas for stuff to work on - there is a lot of work and testing to do with async iterators and Node.js - as well as a lot of interesting work to be done.

Promise.prototype.finally landed - and we should probably test its behavior with unhandled rejections.

The promisify docs (what every method does when promisified) haven't really been touched much since util.promisify landed and it would be great to clarify them and add more examples.

A benchmark for promisify would also be a nice addition - we've done a lot of measurements back before it was landed but we don't currently track performance regressions.

If you'd like help with any of those feel free to reach out or just leave a comment :)

[ramsgoli]

I'd like to take this issue if its still up for grabs! I've never contributed to the node source before but have been looking for ways to get started!

[benjamingr]

@ramsgoli sounds great. here is promisify's code and here are the docs

Here is the general contributing how-to.

Don't hesitate to reach out if you're not sure how to proceed.

[ramsgoli]

@benjamingr Just submitted a PR with these changes!