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

Promisify of method without last argument callback - undefined beahvior #17569

Closed
benjamingr opened this issue Dec 9, 2017 · 9 comments
Closed
Labels
doc Issues and PRs related to the documentations. promises Issues and PRs related to ECMAScript promises.

Comments

@benjamingr
Copy link
Member

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.

@benjamingr benjamingr added doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors. mentor-available promises Issues and PRs related to ECMAScript promises. labels Dec 9, 2017
@mithunsasidharan
Copy link
Contributor

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

@Trott
Copy link
Member

Trott commented Dec 10, 2017

@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
Copy link
Member

Trott commented Dec 10, 2017

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
Copy link
Member

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
Copy link
Contributor

@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
Copy link
Member Author

@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
Copy link
Contributor

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
Copy link
Member Author

@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.

@benjamingr benjamingr removed the good first issue Issues that are suitable for first-time contributors. label Dec 10, 2017
@ramsgoli
Copy link
Contributor

@benjamingr Just submitted a PR with these changes!

ramsgoli added a commit to ramsgoli/node that referenced this issue Dec 12, 2017
Currently the documentation states that promisify() will result in
undefined behavior if bad arguments are passed. This is not necessarily
the case, since the behavior is well defined, but just not useful.

Fixes: nodejs#17569 (comment)
MylesBorins pushed a commit that referenced this issue Jan 8, 2018
Currently the documentation states that promisify() will result in
undefined behavior if bad arguments are passed. This is not
necessarily the case, since the behavior is well defined, but just
not useful.

PR-URL: #17593
Fixes: #17569
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this issue Jan 22, 2018
Currently the documentation states that promisify() will result in
undefined behavior if bad arguments are passed. This is not
necessarily the case, since the behavior is well defined, but just
not useful.

PR-URL: #17593
Fixes: #17569
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. promises Issues and PRs related to ECMAScript promises.
Projects
None yet
Development

No branches or pull requests

5 participants