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(NODE-6174): use client.s.options in session #4134

Merged
merged 6 commits into from
Jun 17, 2024
Merged

Conversation

durran
Copy link
Member

@durran durran commented Jun 6, 2024

Description

What is changing?

When constructing a ClientSession don't use the spread options, instead uses the options directly.

Is there new documentation needed for these changes?

None

What is the motivation for this change?

NODE-6174

Release Highlight

Fill in title or leave empty for no highlight

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: type(NODE-xxxx)[!]: description
    • Example: feat(NODE-1234)!: rewriting everything in coffeescript
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

@nbbeeken
Copy link
Contributor

nbbeeken commented Jun 6, 2024

This is fine for addressing the potential slow down but how would we feel about removing the footgun?

I'm thinking that the Readonly<MongoOptions> return type on the client.options getter is probably "safe enough" such that we could just return client[kOptions] directly. cc @baileympearson

@baileympearson
Copy link
Contributor

I think freezing is nice because it's a runtime guarantee. But why not just freeze the options once at the end of the constructor and make options a property instead of a getter?

@nbbeeken
Copy link
Contributor

nbbeeken commented Jun 6, 2024

That would break the places where we write to the options object, monitorCommands can change at any time, and we stick the TLS values read from files back onto that object. I'm fine with it as is too, just suggesting.

@durran
Copy link
Member Author

durran commented Jun 11, 2024

It is worth noting we do have a unit test that verifies we freeze all public options, so returning kOptions and not freezing it does fail that test. Seems like we just want to stick with the original client.s.options potentially then?

baileympearson
baileympearson previously approved these changes Jun 11, 2024
@durran durran marked this pull request as ready for review June 12, 2024 19:37
@nbbeeken nbbeeken self-assigned this Jun 14, 2024
@nbbeeken nbbeeken changed the title fix(NODE-6174): use client.s.options in session refactor(NODE-6174): use client.s.options in session Jun 14, 2024
@nbbeeken
Copy link
Contributor

LGTM this should clearly have some improvement, so I'll merge this when CI completes. Did we regain the ~9% reported by devtools?

@nbbeeken nbbeeken added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Jun 14, 2024
@nbbeeken nbbeeken merged commit 8eebe6a into main Jun 17, 2024
27 of 29 checks passed
@nbbeeken nbbeeken deleted the NODE-6174 branch June 17, 2024 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Primary Review In Review with primary reviewer, not yet ready for team's eyes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants