Implement Batch query package #2942
Conversation
|
|
||
| The MIT License (MIT) | ||
|
|
||
| Copyright (c) 2013-2020 Ankush Chadda |
There was a problem hiding this comment.
Is it okay to have my name in the copyright ? I am not aware if this is the correct thing or not
There was a problem hiding this comment.
yea absolutely okay. you can get boilerplate MIT license text from github or other places 👍
|
whoah this is really awesome! I'm excited to help get this over the finishline & release it. |
|
@charmander any thoughts on this? I think it looks p great! |
There was a problem hiding this comment.
Some initial comments on things we should change. And needs some tests around errors, error handling, and ensuring the client is still usable after an error (you typically need to flush after an error I believe? Could be wrong there...tests will uncover these things though 😄 )
| } | ||
| catch(err) { | ||
| process.nextTick(() => { | ||
| throw err |
There was a problem hiding this comment.
why catch & throw on next tick? Doesn't this create an uncatchable error which will crash a node process?
| assert.strictEqual(response.rowCount, 1) | ||
| } | ||
| }) | ||
| }) |
There was a problem hiding this comment.
would be good to see some tests for errors / error handling / error recovery
If it’s going to be a separate package instead of an API change to pg’s default query type – why in this repo? |
|
@brianc @charmander please have a look at the PR and provide feedback |
|
I had to make changes in the types/pg to get the build running again |
|
waouh, so happy to see that ! |
There was a problem hiding this comment.
IMHO, it would be great to add explanations about :
- the benefits of batched queries vs non-batched
- the difference with pipelining (which is not implemented yet)
- perhaps also some hints on the underlying mechanism (multiple BIND etc) for future contributors
I also wonder if any advice about the number of parameters is needed ? is there a hard limit ?
|
I think this important question remains unanswered
in other words : are maintainers of this repo willing to take responsability for this new package ? |
@abenhamdine I have followed what @brianc advised me here. |
|
Moving it to draft to add typescript definitions |
Attempts to fix #2257