Skip to content

chore(NODE-6939): update typescript to 5.8.3 #4526

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

chore(NODE-6939): update typescript to 5.8.3 #4526

wants to merge 3 commits into from

Conversation

durran
Copy link
Member

@durran durran commented Apr 30, 2025

Description

Updates typescript to 5.8.3

What is changing?

  • Update typescript.
  • Updates dependencies that were out of date with the TS update.
  • Enable erasableSyntaxOnly option.
  • Fixes all errors due to the new TS option.
  • Fixes all issues arising from the eslint related updates.
Is there new documentation needed for these changes?

None

What is the motivation for this change?

NODE-6939

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

@@ -529,7 +529,7 @@ SINGLETON_TASKS.push(
tags: ['lint-checks'],
commands: [
updateExpansions({
NODE_LTS_VERSION: LOWEST_LTS,
NODE_LTS_VERSION: LATEST_LTS,
Copy link
Member Author

@durran durran Apr 30, 2025

Choose a reason for hiding this comment

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

I made this change since the updated eslint related dependencies will not work on Node 16 (structuredClone isn't available). I figure it was ok to run our lint tasks on latest LTS since it's dev-only and our regular Node 16 tests are all still passing.

Copy link
Contributor

Choose a reason for hiding this comment

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

So long as we still unit test on lowest, I agree that is fine.

@durran durran marked this pull request as ready for review April 30, 2025 15:06
@durran durran requested a review from a team as a code owner April 30, 2025 15:06
@baileympearson baileympearson self-assigned this Apr 30, 2025
@baileympearson baileympearson added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Apr 30, 2025
Copy link
Contributor

@baileympearson baileympearson left a comment

Choose a reason for hiding this comment

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

Just a few small changes

@@ -60,7 +60,7 @@
"source-map-support": "^0.5.21",
"ts-node": "^10.9.2",
"tsd": "^0.31.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

The AC says to update tsd as well

@@ -143,17 +143,15 @@ export class WriteConcern {
const parentOpts: WriteConcern | WriteConcernSettings | undefined =
inherit instanceof WriteConcern ? inherit : inherit.writeConcern;

const mergedOpts = { ...parentOpts, ...opts } as WriteConcernSettings;
const {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const {
const mergedOpts: WriteConcernSettings = { ...parentOpts, ...opts };

avoid casting

Copy link
Member Author

Choose a reason for hiding this comment

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

I needed to cast here as the parentOps and opts had potential undefined values.

@@ -605,6 +605,7 @@ function setOption(
if (values[0] == null) {
break;
}
// eslint-disable-next-line @typescript-eslint/no-base-to-string
Copy link
Contributor

Choose a reason for hiding this comment

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

suggest: add a comment explaining why we want to ignore this lint warning

Copy link
Member Author

Choose a reason for hiding this comment

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

Added comment.

@@ -96,7 +96,7 @@ export class DistinctOperation extends CommandOperation<any[]> {

const result = await super.executeCommand(server, session, cmd, timeoutContext);

return this.explain ? result : result.values;
return this.explain ? (result as any[]) : result.values;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return this.explain ? (result as any[]) : result.values;
// @ts-expect-error Explain always returns a document
return this.explain ? result : result.values;

Instead of hiding the casting away the here, we should make it clear we expect this TS error and explain why

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated.

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.

2 participants