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

fix(NODE-4977): load snappy lazily #3726

Merged
merged 14 commits into from Jul 3, 2023
Merged

fix(NODE-4977): load snappy lazily #3726

merged 14 commits into from Jul 3, 2023

Conversation

nbbeeken
Copy link
Contributor

@nbbeeken nbbeeken commented Jun 14, 2023

Description

What is changing?

  • added getSnappy
  • use it in the compression path so its imported lazily
  • added a test to assert a small hardcoded list of deps upon importing the driver
  • added a test that launches a new node process to test lazy importing
    • wrote a utility to structure this like a mocha test
Is there new documentation needed for these changes?

Perhaps, when we've completed the related investigations we should capture out approach to optional importing and how it can effect bundling.

What is the motivation for this change?

We only want to import an optional dependency if our library is actually using it. Snappy may be present for other reasons, and node's dependency resolution doesn't differentiate if an optional peer installed is for "our" need or for coincidentally another sibling package.

Release Highlight

Snappy loaded lazily

Unlike our other compression mechanisms snappy was loaded at the module level, meaning it would be optionally imported whether or not the driver was configured to use snappy compression. Snappy is now aligned with our other optional peer dependencies and is only loaded when enabled.

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 nbbeeken marked this pull request as ready for review June 16, 2023 17:01
@nbbeeken nbbeeken changed the title fix(NODE-4977): ensure snappy is loaded lazily fix(NODE-4977): load snappy lazily Jun 16, 2023
@durran durran self-assigned this Jun 20, 2023
@durran durran added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Jun 20, 2023
test/tools/utils.ts Outdated Show resolved Hide resolved
test/tools/utils.ts Outdated Show resolved Hide resolved
@nbbeeken nbbeeken requested a review from addaleax June 20, 2023 17:15
@addaleax
Copy link
Contributor

@nbbeeken I don’t think I have the time for a full review right now :)

@durran durran added Team Review Needs review from team and removed Primary Review In Review with primary reviewer, not yet ready for team's eyes labels Jun 20, 2023
src/deps.ts Outdated Show resolved Hide resolved
@durran durran merged commit 865e658 into main Jul 3, 2023
24 of 27 checks passed
@durran durran deleted the NODE-4977-snappy-lazy branch July 3, 2023 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team Review Needs review from team
Projects
None yet
4 participants