-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(NODE-5545): remove deprecated objects #4704
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few minor comments!
socketTimeoutMS?: number; | ||
/** @internal */ | ||
cancellationToken?: CancellationToken; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably fine because I don't expect users to actually be using this type. But want to confirm we're okay making this field internal, even though it isn't deprecated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's kind of annoying that this is a public type at all - best I can tell it's all for the supported tls options list; I wonder if we could invert our types for the next major where this interface can be private, composed of the public options plus whatever else we need internally...
I think we should make a note of this so that we can consider releasing a minor version with the deprecation before the major, but I don't think we need to commit to actually doing it for just this one thing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated this to deprecated now with a note on what to use instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant make a note outside this PR (e.g. a follow up ticket that we can consider doing or not when we get closer to release time)... we should remove it in this PR.
@baileympearson #4704 (comment) I've updated the GridFS tests to latest and skipped the new ones. |
socketTimeoutMS?: number; | ||
/** @internal */ | ||
cancellationToken?: CancellationToken; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's kind of annoying that this is a public type at all - best I can tell it's all for the supported tls options list; I wonder if we could invert our types for the next major where this interface can be private, composed of the public options plus whatever else we need internally...
I think we should make a note of this so that we can consider releasing a minor version with the deprecation before the major, but I don't think we need to commit to actually doing it for just this one thing
// This is a regression test that we don't remove the unused generic in FindOptions | ||
const findOptions: FindOptions<{ a: number }> = {}; | ||
const findOptions: FindOptions = {}; | ||
expectType<FindOptions>(findOptions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can just be deleted - the comment above says the intention is to make sure we don't remove the generic - we are now intentionally removing the generic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
Co-authored-by: Bailey Pearson <[email protected]>
Co-authored-by: Bailey Pearson <[email protected]>
Co-authored-by: Bailey Pearson <[email protected]>
Description
Removes deprecated items or marks them internal where appropriate.
Summary of Changes
What is the motivation for this change?
NODE-5545
Release Highlight
Deprecated Types, Classes, and Options Have Been Removed
Includes:
Double check the following
npm run check:lint
)type(NODE-xxxx)[!]: description
feat(NODE-1234)!: rewriting everything in coffeescript