feat(NODE-4547): mark all callback APIs as deprecated #3388
Conversation
baileympearson
added
the
Primary Review
There was a problem hiding this comment.
I went through the PR and for every method listed in api.js in the legacy repo, I made sure it was deprecated in the driver. I then deleted it from my local copy of api.js. Afterwards, I was left with this:
/* eslint-disable prettier/prettier */
const { byStrings, sorted } = require('./utils');
module.exports = Object.create(null);
Object.defineProperty(module.exports, '__esModule', { value: true });
const commonCursorApis = [
{ className: 'AbstractCursor', method: 'close', returnType: 'Promise<void>' },
{ className: 'AbstractCursor', method: 'forEach', returnType: 'Promise<void>', possibleCallbackPositions: [1] },
{ className: 'AbstractCursor', method: 'hasNext', returnType: 'Promise<boolean>' },
{ className: 'AbstractCursor', method: 'next', returnType: 'Promise<TSchema | null>' },
{ className: 'AbstractCursor', method: 'toArray', returnType: 'Promise<TSchema[]>' },
{ className: 'AbstractCursor', method: 'tryNext', returnType: 'Promise<TSchema | null>' },
]
module.exports.commonCursorApis = commonCursorApis;
const cursorClasses = [
'FindCursor',
'AggregationCursor',
'ListIndexesCursor',
'ListCollectionsCursor'
]
module.exports.cursorClasses = cursorClasses;
const api = [
// Super class of cursors, we do not directly override these but override them in the inherited classes
...commonCursorApis.flatMap(({ method, returnType, possibleCallbackPositions }) => cursorClasses.map(cursorClass => ({ className: cursorClass, method, returnType, possibleCallbackPositions }))),
{ className: 'Admin', method: 'removeUser', returnType: 'Promise<boolean>' },
{ className: 'AggregationCursor', method: 'explain', returnType: 'Promise<Document>' },
{ className: 'ChangeStream', method: 'close', returnType: 'Promise<void>' },
{ className: 'FindCursor', method: 'count', returnType: 'Promise<number>' },
{ className: 'FindCursor', method: 'explain', returnType: 'Promise<Document>' },
// Manually test the static version of connect
// This is listed here as a reference for completeness, but it is tested manually
// it is checked to exist in index.test.js
// its functionally tested in maybe_callback.test.js
// { className: 'MongoClient', method: 'static connect', returnType: 'Promise<this>' },
];
module.exports.api = api;
module.exports.classNames = new Set(api.map(({className}) => className))
module.exports.classNameToMethodList = new Map(api.map((api, _, array) =>
[api.className, sorted(Array.from(new Set(Array.from(array.filter(v => v.className === api.className), method => method))), (a, b) => byStrings(a.method, b.method))]
));It looks like we're missing the cursor apis and a few misc functions.
| countDocuments(callback: Callback<number>): void; | ||
| countDocuments(filter: Filter<TSchema>): Promise<number>; | ||
| /** @deprecated Callbacks are deprecated and will be removed in the next major version */ | ||
| countDocuments(callback: Callback<number>): void; |
There was a problem hiding this comment.
can we remove this duplicate overload?
There was a problem hiding this comment.
We can fix it in another PR, this commit will be docs changes only currently, I would prefer to not add type changes -- esp since we have at least two things to fix
There was a problem hiding this comment.
It's not a type change if it's a duplicate overload. it's literally a duplicate of line 1145
There was a problem hiding this comment.
An overload has is emitted to the definitions, removing it changes how the defintions resolve the correct call format. I agree we should remove it, I just want this PR to involve only docs changes, the plan is to generate the docs pages when we're done checking that the @\deprecation tag is in all the right places
There was a problem hiding this comment.
This won't affect function call resolution at all - if that overload is matched, it will match the first instance of that overload. If not, it will never match the second.
Can you explain the plan about generating the docs more? Will that be included in this PR or in follow up work?
There was a problem hiding this comment.
In this PR I'll pull in the generated docs changes so we can merge it all together, but GH diff is difficult with many changes so I'm saving that for when the review is complete
Yea that's correct about the behavior at least to my understanding
src/collection.ts
Outdated
| @@ -1582,7 +1647,7 @@ export class Collection<TSchema extends Document = Document> { | |||
| * one will be added to each of the documents missing it by the driver, mutating the document. This behavior | |||
| * can be overridden by setting the **forceServerObjectId** flag. | |||
| * | |||
| * @deprecated Use insertOne, insertMany or bulkWrite instead. | |||
| * @deprecated Use insertOne, insertMany or bulkWrite instead. Callbacks are deprecated and will be removed in the next major version | |||
There was a problem hiding this comment.
This comment applies to any previously deprecated functions that take callbacks in this PR.
How do want to handle these cases? I couldn't find an existing ticket, so as far as I can tell we have no plans to remove these functions in 5.0. My preference would be to remove these functions, and if so, I think we should 1. file a jira ticket for their removal and make sure it's tagged for 5.0 and 2. remove the callback specific deprecation.
If we don't decide to go this route - I think we should properly overload all these functions and deprecate the callback overloads specifically, mentioning their removal in v5.
Thoughts?
There was a problem hiding this comment.
There was a problem hiding this comment.
Can we remove the callback deprecation warning here as well?
There was a problem hiding this comment.
We should mention both but maybe I could rephrase? since currently migrating off of .insert is
- c.insert({ doc: 1 }, callback)
+ c.insertMany([{ doc: 1 }], callback)Now that callbacks will be removed in addition it is more involved than just wrapping the first argument in brackets
There was a problem hiding this comment.
I still think we should only mention the whole deprecation. insert (and similar functions) will be entirely removed in v5. I think the user flow of
- user sees insert is deprecated and switches to insertOne
- user sees insertOne with callbacks is deprecated and switches to promises
is fine because at each stage, they see an specific deprecation warning and know how to resolve it. a single warning that combines both (i.e., Use insertOne, insertMany or bulkWrite instead. Callbacks are deprecated and will be removed in the next major version) is more confusing. Maybe we should ask the team / Matt
There was a problem hiding this comment.
from the conversation in slack, callback deprecation and removal is distinct and more certain than the whole api deprecation and removal, we should have separate notes so that the corresponding tasks can be completed separately
baileympearson
added
Team Review
Description
What is changing?
What is the motivation for this change?
Double check the following
npm run check:lintscript<type>(NODE-xxxx)<!>: <description>