fix(mongodb): correct GridFSBucketWriteStream.abort callback#44553
fix(mongodb): correct GridFSBucketWriteStream.abort callback#44553typescript-bot merged 1 commit intoDefinitelyTyped:masterfrom
Conversation
|
@peterblazejewicz Thank you for submitting this PR! Code ReviewsBecause you edited one package and updated the tests (👏), I can help you merge this PR once someone else signs off on it. Status
All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes. Diagnostic Information: What the bot saw about this PR{
"type": "info",
"now": "-",
"pr_number": 44553,
"author": "peterblazejewicz",
"owners": [
"CaselIT",
"alanmarcell",
"dante-101",
"mcortesi",
"EnricoPicci",
"AJCStriker",
"julien-c",
"daprahamian",
"denys-bushulyak",
"BastienAr",
"sindbach",
"geraldinelemeur",
"various89",
"angela-1",
"lirbank",
"hector7",
"floric",
"erikc5000",
"Manc",
"jloveridge",
"ranguna",
"HosseinAgha",
"albertossilva",
"peterblazejewicz",
"LinusU",
"taxilian",
"xamgore",
"avaly",
"HitkoDev",
"jtassin"
],
"dangerLevel": "ScopedAndTested",
"headCommitAbbrOid": "b78ef10",
"headCommitOid": "b78ef10efc42b6c23a2123c6527dec011b57f51d",
"mergeIsRequested": true,
"stalenessInDays": 0,
"lastCommitDate": "2020-05-18T18:36:28.000Z",
"lastCommentDate": "2020-05-18T18:42:57.000Z",
"reviewLink": "https://github.com/DefinitelyTyped/DefinitelyTyped/pull/44553/files",
"hasMergeConflict": false,
"authorIsOwner": true,
"isFirstContribution": false,
"popularityLevel": "Popular",
"anyPackageIsNew": false,
"packages": [
"mongodb"
],
"files": [
{
"filePath": "types/mongodb/index.d.ts",
"kind": "definition",
"package": "mongodb"
},
{
"filePath": "types/mongodb/test/index.ts",
"kind": "test",
"package": "mongodb"
}
],
"hasDismissedReview": false,
"travisResult": "pass",
"lastReviewDate": "2020-05-18T18:41:49.000Z",
"reviewersWithStaleReviews": [
{
"reviewedAbbrOid": "d4d38c2",
"reviewer": "LinusU",
"date": "2020-05-13T17:22:56Z"
}
],
"approvalFlags": 2,
"isChangesRequested": false
} |
|
🔔 @CaselIT @alanmarcell @Dante-101 @mcortesi @EnricoPicci @AJCStriker @julien-c @daprahamian @Denys-Bushulyak @bastienar @sindbach @geraldinelemeur @various89 @angela-1 @lirbank @hector7 @floric @erikc5000 @Manc @jloveridge @Ranguna @HosseinAgha @albertossilva @LinusU @taxilian @xamgore @avaly @HitkoDev @jtassin - please review this PR in the next few days. Be sure to explicitly select |
|
👋 Hi there! I’ve run some quick measurements against master and your PR. These metrics should help the humans reviewing this PR gauge whether it might negatively affect compile times or editor responsiveness for users who install these typings. Let’s review the numbers, shall we? Comparison details 📊
It looks like nothing changed too much. I won’t post performance data again unless it gets worse. |
|
This looks like an improvement to me, but I don't like merging changes to such a widely used package without some authors signing off. @CaselIT @alanmarcell @Dante-101 @mcortesi @EnricoPicci @AJCStriker @julien-c @daprahamian @Denys-Bushulyak @bastienar @sindbach @geraldinelemeur @various89 @angela-1 @lirbank @hector7 @floric @erikc5000 @Manc @jloveridge @Ranguna @HosseinAgha @albertossilva @LinusU @taxilian @xamgore @avaly @HitkoDev |
|
I believe you should bump the package version, as it looks like a minor breaking change. |
From the added test it seems to not break the older callback |
|
It seems most functions should only accept https://mongodb.github.io/node-mongodb-native/3.6/api/lib_gridfs-stream_upload.js.html#line112 |
types/mongodb/index.d.ts
Outdated
| (err?: MongoError): void; | ||
| /** http://mongodb.github.io/node-mongodb-native/3.6/api/GridFSBucket.html#~errorCallback */ | ||
| export interface GridFSBucketErrorCallback<T = any> extends MongoCallback<T> { | ||
| (error: MongoError, result: T): void; |
There was a problem hiding this comment.
Since you removed ? I think that you should make this | undefined. The same goes for T since it won't be available when error is.
| (error: MongoError, result: T): void; | |
| (error: MongoError | undefined, result: T | undefined): void; |
Maybe even, if it works:
| (error: MongoError, result: T): void; | |
| (error: undefined, result: T): void; | |
| (error: MongoError, result: undefined): void; |
There was a problem hiding this comment.
Actually, since this is only used in one place (right?), I think it would be better to just:
| (error: MongoError, result: T): void; | |
| (error?: MongoError | null): void; |
also here, I think that null is actually the value received, not undefined. I might be wrong though
|
Sorry for the late review, added som comments now. Basically I think that the best approach would be: export interface GridFSBucketErrorCallback extends MongoCallback<void> {}together with a modification of export interface MongoCallback<T> {
(error: MongoError | null, result?: T | null): void;
} |
|
Let me see, be back with feedback/changes
absolutely not a problem nowadays |
It seems it would be impacting a lot of existing code, e.g.: (err: mongodb.MongoError, client: mongodb.MongoClient) => {This patch passses all tests (inluding referenced libs) diff --git a/types/mongodb/index.d.ts b/types/mongodb/index.d.ts
index 1ca7230faa..b609a248a5 100644
--- a/types/mongodb/index.d.ts
+++ b/types/mongodb/index.d.ts
@@ -2439,10 +2439,8 @@ export interface GridFSBucketOptions {
readPreference?: ReadPreferenceOrMode;
}
-/** http://mongodb.github.io/node-mongodb-native/3.1/api/GridFSBucket.html#~errorCallback */
-export interface GridFSBucketErrorCallback {
- (err?: MongoError): void;
-}
+/** http://mongodb.github.io/node-mongodb-native/3.6/api/GridFSBucket.html#~errorCallback */
+export interface GridFSBucketErrorCallback extends MongoCallback<void> {}
/** http://mongodb.github.io/node-mongodb-native/3.1/api/GridFSBucket.html#find */
export interface GridFSBucketFindOptions {
@@ -2487,7 +2485,7 @@ export class GridFSBucketWriteStream extends Writable {
* @param [callback] called when chunks are successfully removed or error occurred
* @see {@link https://mongodb.github.io/node-mongodb-native/3.6/api/GridFSBucketWriteStream.html#abort}
*/
- abort(callback?: () => void): void;
+ abort(callback?: GridFSBucketErrorCallback): void;
}
/** https://mongodb.github.io/node-mongodb-native/3.1/api/GridFSBucketWriteStream.html */
diff --git a/types/mongodb/test/index.ts b/types/mongodb/test/index.ts
index a8b0645d31..93576621d0 100644
--- a/types/mongodb/test/index.ts
+++ b/types/mongodb/test/index.ts
@@ -89,10 +89,14 @@ const gridFSBucketTests = (bucket: mongodb.GridFSBucket) => {
const openUploadStream = bucket.openUploadStream('file.dat');
openUploadStream.on('close', () => {});
openUploadStream.on('end', () => {});
+ openUploadStream.abort(); // $ExpectType void
openUploadStream.abort(() => {
openUploadStream.removeAllListeners();
});
- openUploadStream.abort();
+ openUploadStream.abort((error) => {
+ error; // $ExpectType MongoError
+ });
+ openUploadStream.abort((error, result) => {});
};
// Compressionso, as you can see, only small, focused change to the callback interface for this specific stream type. |
Yes, but I think that that would be good. Currently the typings here are basically turning off That being said, if you just want this PR to focus on changing the specific callback for |
Yes, my exact intent. I'll amend with the patch described. |
This fixes callback method definition to match documentation: https://mongodb.github.io/node-mongodb-native/3.6/api/GridFSBucket.html#~errorCallback Also this error callback now has the same shape as default MongoCallback, hence the extend. /cc @alexy4744 Fixes DefinitelyTyped#44549
d4d38c2 to
b78ef10
Compare
|
@CaselIT Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review? |
|
rebased, with minor changes (as per @LinusU feedback) |
|
@LinusU Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review? |
|
Ready to merge |
|
I just published |
…iteStream.abort callback by @peterblazejewicz This fixes callback method definition to match documentation: https://mongodb.github.io/node-mongodb-native/3.6/api/GridFSBucket.html#~errorCallback Also this error callback now has the same shape as default MongoCallback, hence the extend. /cc @alexy4744 Fixes DefinitelyTyped#44549
…iteStream.abort callback by @peterblazejewicz This fixes callback method definition to match documentation: https://mongodb.github.io/node-mongodb-native/3.6/api/GridFSBucket.html#~errorCallback Also this error callback now has the same shape as default MongoCallback, hence the extend. /cc @alexy4744 Fixes DefinitelyTyped#44549
This fixes callback method definition to match documentation:
https://mongodb.github.io/node-mongodb-native/3.6/api/GridFSBucket.html#~errorCallback
Also this error callback now has the same shape as default
MongoCallback, hence the extend.
/cc @alexy4744
Fixes #44549
npm test.)npm run lint package-name(ortscif notslint.jsonis present).