feat: add callbackify() and callbackifyAll() methods #82
Conversation
googlebot
added
the
cla: yes
src/index.ts
Outdated
| .then((res: []) => { | ||
| cb(null, ...res); | ||
| }) | ||
| .catch((err: Error) => cb(err)); |
There was a problem hiding this comment.
This is a bug. The error handler should be passed as the second parameter of the then method. That prevents errors thrown in the callback method from triggering the catch clause, and double calling the callback.
src/index.ts
Outdated
| exports.callbackify(originalMethod, options); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
please add a newline
| }); | ||
| }); | ||
|
|
||
| it('should call the callback with error when promise rejects', () => { |
There was a problem hiding this comment.
Please add a "it should call the callback only a single time when the promise rejects" to test the error we caught above
There was a problem hiding this comment.
added a test 'should call the callback only a single time when the promise resolves but callback throws an error', however skipping the test as of now as I am unable to catch the error thrown by the callback in the test.
There was a problem hiding this comment.
I'm a little confused. Why do we need to skip the test? I really think we need to have that enabled.
There was a problem hiding this comment.
the error that is thrown by the callback is crushing the test. I am not able to catch /intercept it.
I tried assert.throws, try=>catch block around func(callback), process.on('uncaughtException'), process.on('unhandledRejection').
There was a problem hiding this comment.
Any suggestions would be greatly appreciated.
There was a problem hiding this comment.
I believe the bug was only in case when promise resolves, but callback throws an error, then callback is called again with that error in the catch block.
There was a problem hiding this comment.
Ah, so in that scenario it would be a user error, I misunderstood. I wonder if we would want to change our logic to look something like this
originalMethod
.apply(context, arguments)
// tslint:disable-next-line:no-any
.then((res: any) => {
res = Array.isArray(res) ? res : [res];
cb(null, ...res);
}, (err: Error) => cb(err))
.catch(err => process.emitWarning(err));There was a problem hiding this comment.
I fill that if error is thrown by the user's function, it should be propagated and the process should crush, no?
There was a problem hiding this comment.
Yeah, the native callbackify doesn't do anything special with the error either. So I don't think we shouldn't concern ourselves with user errors like that.
There was a problem hiding this comment.
Caught that error 👍
Test is enabled.
JustinBeckwith
changed the title
|
Added changes for scenarios when promise returns not |
src/index.ts
Outdated
| const originalMethod = Class.prototype[methodName]; | ||
| if (!originalMethod.callbackified_) { | ||
| Class.prototype[methodName] = | ||
| exports.callbackify(originalMethod, options); |
There was a problem hiding this comment.
It doesn't look like callbackify actually accepts any options
| }); | ||
| }); | ||
|
|
||
| it('should call the callback with error when promise rejects', () => { |
There was a problem hiding this comment.
I think instead of a separate test, you could modify the current test to look like this
const error = new Error('err');
func = util.callbackify(async () => {
throw error;
});
func((err: Error) => {
assert.strictEqual(err, error);
done();
});If done gets called multiple times the test will fail, so I think that would satisfy @JustinBeckwith's ask (maybe?)
after promise resolves
Codecov Report
@@ Coverage Diff @@
## master #82 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 1 1
Lines 34 54 +20
Branches 5 8 +3
=====================================
+ Hits 34 54 +20
Continue to review full report at Codecov.
|
README.md@google-cloud/promisify