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

Reconsider making mappingHint's default mapper wrap in an array. #360

Closed
vqvu opened this issue Aug 7, 2015 · 1 comment
Closed

Reconsider making mappingHint's default mapper wrap in an array. #360

vqvu opened this issue Aug 7, 2015 · 1 comment

Comments

@vqvu
Copy link
Collaborator

vqvu commented Aug 7, 2015

Some background:
#247 added mappingHint to wrapCallback. At the time, we thought that the default behavior should wrap multiple arguments to the callback in an array, but this was not backwards compatible, so we decided to delay until 3.0 and added a note in the docs warning of the impending change.

I'm not so sure this default behavior is a good thing anymore. See petkaantonov/bluebird#307 for the issues bluebird (and Q) have had with this default behavior breaking promisified functions. Basically, the following cases can happen

  1. Library writer adds an extra argument to the callback. From their standpoint, this is not a breaking change, but it causes wrapCallback to push an array instead.
  2. Library writer normally fires the callback with two arguments, but adds a code path that only does it with one (e.g., cb(null, value) instead of cb(null, value, null)). The intention is that the second value should be null. This is a common Javascript pattern, but it causes wrapCallback to switch between pushing an array and not practically willy-nilly. This is very bad.

I suggest we take out the wording in the mappingHint docs and just ask people to specify a mappingHint when they need to.

@vqvu
Copy link
Collaborator Author

vqvu commented Sep 2, 2015

Closing this since #335 exists, and discussions should go there instead.

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

No branches or pull requests

1 participant