Repository navigation
fix: promise never returned on table.insert - #953
Conversation
Codecov Report
@@ Coverage Diff @@
## master #953 +/- ##
==========================================
- Coverage 98.68% 98.66% -0.03%
==========================================
Files 9 9
Lines 7173 7176 +3
Branches 442 487 +45
==========================================
+ Hits 7079 7080 +1
- Misses 94 96 +2
Continue to review full report at Codecov.
|
feywind
left a comment
There was a problem hiding this comment.
I disagree with codecov, I think those lines are covered.
| err => callback(err, null) | ||
| ); | ||
| } else { | ||
| return promise.then(r => [r]); |
There was a problem hiding this comment.
It's sort of odd to me that the Promise returns an array, but the callback isn't called with one. This seems to maintain the method definition though.
There was a problem hiding this comment.
Or it could be typescript having trouble determining the return type.
I'm not sure if there's a test for the callback version's return type (there probably should be).
| err => callback(err, null) | ||
| ); | ||
| const promise = this._insertAndCreateTable(rows, options); | ||
| if (callback) { |
There was a problem hiding this comment.
I don't think this is needed, as there's already a promisified version being returned. See
Line 2388 in a138347
I think the unhandled rejection may come from further down.
There was a problem hiding this comment.
You're right, this PR fixes nothing then. Easy to miss. Not the biggest fan of code like that tbh.
cc @feywind
🤖 I have created a release \*beep\* \*boop\* --- ## [5.7.0](https://www.github.com/googleapis/nodejs-bigquery/compare/v5.6.0...v5.7.0) (2021-07-21) ### Features * add `gcf-owl-bot[bot]` to `ignoreAuthors` ([#958](https://www.github.com/googleapis/nodejs-bigquery/issues/958)) ([9eab78d](https://www.github.com/googleapis/nodejs-bigquery/commit/9eab78de56d087f3dc756ab2c0974737a80ad43f)) ### Bug Fixes * **deps:** update dependency yargs to v17 ([#954](https://www.github.com/googleapis/nodejs-bigquery/issues/954)) ([28cf08d](https://www.github.com/googleapis/nodejs-bigquery/commit/28cf08d74184b388cd8d18a9622630939c5f99cf)) * extend definition of Query.types for simple named parameters ([#906](https://www.github.com/googleapis/nodejs-bigquery/issues/906)) ([#907](https://www.github.com/googleapis/nodejs-bigquery/issues/907)) ([44e1ac7](https://www.github.com/googleapis/nodejs-bigquery/commit/44e1ac7cf8604d79508316d70a3a98e2953d59f0)) * handle null query parameter value ([#920](https://www.github.com/googleapis/nodejs-bigquery/issues/920)) ([3bf900a](https://www.github.com/googleapis/nodejs-bigquery/commit/3bf900a54a92c0422fa8f3c48480dc430a7d134d)) * promise never returned on table.insert ([#953](https://www.github.com/googleapis/nodejs-bigquery/issues/953)) ([a138347](https://www.github.com/googleapis/nodejs-bigquery/commit/a138347855f74d4e5c889dc42b00992c4a3808a6)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Fixes #948
So the
table.insertfunction never returned its promise. This PR fixes that.It also gets rid of an unnecessary cast.
Two new tests were written:
it should return a promise if no callback is providedit should resolve to an array on successCode coverage goes down to 99.92, due to line 1916 being uncovered (even though thats what the new tests were for).