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

chore(NODE-5323): update to typescript 5 #3694

Merged
merged 5 commits into from
Jun 2, 2023
Merged

chore(NODE-5323): update to typescript 5 #3694

merged 5 commits into from
Jun 2, 2023

Conversation

durran
Copy link
Member

@durran durran commented May 31, 2023

Description

Updates the driver to use Typescript 5.

What is changing?

  • Updates typescript to ^5.0.4
  • Updates api-extractor to ^7.35.1 (to use typescript 5)
  • Updates tsd to ^0.28.1
  • All other typescript dependencies were already further along than the minimum.
  • Removes the importsNotUsedAsValues config and replaces with eslint rule.
  • Updates all the files to adhere to new eslint rule.
  • Fixes the number comparison between number and Int32.
Is there new documentation needed for these changes?

None

What is the motivation for this change?

NODE-5323

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

@@ -17,7 +17,6 @@
// Never emit error filled code
"noEmitOnError": true,
"outDir": "lib",
"importsNotUsedAsValues": "error",
Copy link
Member Author

Choose a reason for hiding this comment

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

This is deprecated, now using eslint rule.

@@ -88,6 +88,14 @@
],
"@typescript-eslint/no-explicit-any": "off",
"@typescript-eslint/require-await": "off",
"@typescript-eslint/consistent-type-imports": [
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the new rule replacing the deprecated importsNotUsedAsValues typescript config.

src/cmap/connect.ts Outdated Show resolved Hide resolved
@durran durran marked this pull request as ready for review May 31, 2023 20:16
@nbbeeken nbbeeken self-assigned this Jun 1, 2023
@nbbeeken nbbeeken added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Jun 1, 2023
src/cmap/connect.ts Outdated Show resolved Hide resolved
@durran durran requested a review from nbbeeken June 1, 2023 20:37
Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

one last improvement

hello &&
typeof hello.maxWireVersion?.valueOf() === 'number' &&
hello.maxWireVersion >= MIN_SUPPORTED_WIRE_VERSION;
hello && !Number.isNaN(maxWireVersion) && hello.maxWireVersion >= MIN_SUPPORTED_WIRE_VERSION;
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
hello && !Number.isNaN(maxWireVersion) && hello.maxWireVersion >= MIN_SUPPORTED_WIRE_VERSION;
!Number.isNaN(maxWireVersion) && hello.maxWireVersion >= MIN_SUPPORTED_WIRE_VERSION;

If hello is nullish which I don't think it can be then the lines 82/83 will throw, so we can remove this check

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.

@durran durran requested a review from nbbeeken June 2, 2023 14:24
@nbbeeken nbbeeken merged commit 424f133 into main Jun 2, 2023
@nbbeeken nbbeeken deleted the NODE-5323 branch June 2, 2023 15:50
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