Skip to content
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

refactor!: remove deprecated items #2740

Merged
merged 3 commits into from
Feb 19, 2021
Merged

Conversation

nbbeeken
Copy link
Contributor

Removals are noted in JIRA ticket.

NODE-2317

Some of the deprecations are not to be removed (e.g, Collection#insert) but a warning should still be emitted I wrote a small wrapper around process.emitWarning that we should also port to 3.6. This allows us to drop the deprecate calls that come after the class definition (e.g, deprecate(Collection.prototype.insert, ...)) and move a emitWarningOnce call into the method itself.

@nbbeeken nbbeeken force-pushed the NODE-2317/removeDeprecations branch 6 times, most recently from 3b45cef to b7cb028 Compare February 17, 2021 16:25
@nbbeeken nbbeeken marked this pull request as ready for review February 17, 2021 17:18
Copy link
Contributor

@emadum emadum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, very nice to see all that legacy code removed! 👍

src/cmap/commands.ts Outdated Show resolved Hide resolved
src/operations/add_user.ts Outdated Show resolved Hide resolved
* so using string interpolation can cause multiple emits
* @internal
*/
export function emitWarningOnce(message: string): void {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, this was exactly what I was thinking we should do 🥇

Copy link
Member

@durran durran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@nbbeeken
Copy link
Contributor Author

nbbeeken commented Feb 19, 2021

@addaleax These are the deprecations in 3.6 that are slated for removal in 4.0, there are failures with the mongosh tests. If you (or a colleague) get the chance, could you take a look and give us some insight into what is expected/unexpected here.

I'll spare pinging you twice but same goes for: #2741

@addaleax
Copy link
Contributor

If you (or a colleague) get the chance, could you take a look and give us some insight into what is expected/unexpected here.

It's failing during typescript compilation in both cases (I know the test logs can be a bit messy -- it's generally helpful to grep around for the first npm ERR! if you're looking at them to find the source of the problem(s)):

 [2021/02/19 18:27:59.938] TSError: ⨯ Unable to compile TypeScript:
 [2021/02/19 18:27:59.938] src/cli-service-provider.spec.ts(295,22): error TS2339: Property 'findAndModify' does not exist on type 'StubbedInstance<Collection>'.
 [2021/02/19 18:27:59.938] src/cli-service-provider.spec.ts(308,29): error TS2339: Property 'findAndModify' does not exist on type 'StubbedInstance<Collection>'.

and for #2741

 [2021/02/19 18:42:58.456] src/shell-internal-state.ts(223,29): error TS2749: 'TopologyType' refers to a value, but is being used as a type here. Did you mean 'typeof TopologyType'?

The latter is probably not hard to address, you can merge it and we'll probably really just have to add typeof in a places where we're referring to the type of what-was-once-an-enum.

For the findAndModify one, I don't think we were aware that it was deprecated. It's exposed as a shell method, so we'll need to find a way of implementing it in terms of the methods that replace it, which doesn't seem hard to me, but I haven't tried it yet so I'm not sure.

And thanks for the ping! :)

@nbbeeken
Copy link
Contributor Author

For the findAndModify one, I don't think we were aware that it was deprecated. It's exposed as a shell method, so we'll need to find a way of implementing it in terms of the methods that replace it, which doesn't seem hard to me, but I haven't tried it yet so I'm not sure.

Thanks for going through the logs! Sounds like we're good to go, of course let us know if we can advise/help out with the findAndModify patch. There are three options that can be passed to findAndModify:

  • update?: boolean;
  • remove?: boolean;
  • upsert?: boolean;

and that's what makes it do the same thing as findOneAndUpdate, findOneAndDelete, findOneAndReplace.

@nbbeeken nbbeeken merged commit ee1a4d3 into 4.0 Feb 19, 2021
@nbbeeken nbbeeken deleted the NODE-2317/removeDeprecations branch February 19, 2021 22:19
ljhaywar pushed a commit that referenced this pull request Nov 9, 2021
Removals are noted in JIRA ticket.

NODE-2317
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants