Handle throws in type parsers #1218
Conversation
lib/query.js
Outdated
| row = this._result.parseRow(msg.fields); | ||
| } catch (err) { | ||
| if(this.callback) { | ||
| return this.callback(err); |
There was a problem hiding this comment.
This will call the callback at least twice if there are errors: once for each errored row, and then once to incorrectly indicate a successful completion, with errored rows missing.
There was a problem hiding this comment.
Good catch, looking into it now 👍
|
@charmander should be fixed now :) |
LinusU
force-pushed
the
throw-in-type-parser
branch
from
f309a21 to
8a01e1d
Compare
LinusU
force-pushed
the
throw-in-type-parser
branch
from
8a01e1d to
a0c341c
Compare
|
@charmander Studied the code more carefully and came up with what I think is a more proper solution. This should now handle the errors correctly and the tests should test it properly (e.g. was missing |
| test('handles throws in type parsers', function() { | ||
|
|
||
| types.setTypeParser('special oid that will throw', function () { | ||
| throw Object.assign(new Error('Test'), { throwInTypeParser: true }); |
There was a problem hiding this comment.
Object.assign isn’t compatible with Node ≤ 0.12.
There was a problem hiding this comment.
It might be best to create the error outside the type parser and check that it’s equal to the emitted error rather than adding throwInTypeParser.
There was a problem hiding this comment.
Very smart, I'll fix that today 👌
I don't think this failure is related to my commit :) restarting the build... |
LinusU
force-pushed
the
throw-in-type-parser
branch
from
703f12d to
fd30e95
Compare
|
🤔 same error again, seems like something is broken on Travis or the Node.js releases server... |
LinusU
force-pushed
the
throw-in-type-parser
branch
from
fd30e95 to
2c4b363
Compare
|
...and the build passes! 🎉 🎉 |
|
ping @brianc, any chance of getting this merged? :) |
|
ping @brianc, I would really appreciate it if we could merge this one 🙌 |
|
Nevermind - I see what's going on now - had to read the entire query file. |
|
Publishing new patch version to npm now - sorry for the delay. |
|
Thank you ❤️ |
Fixes #1204