Skip to content

Optional deps#453

Open
KoblerS wants to merge 9 commits into
mainfrom
optional-deps
Open

Optional deps#453
KoblerS wants to merge 9 commits into
mainfrom
optional-deps

Conversation

@KoblerS

@KoblerS KoblerS commented May 26, 2026

Copy link
Copy Markdown
Member

Make Cloud Storage SDKs Optional Peer Dependencies (Breaking Change v4.0.0)

New Feature / Refactor

♻️ Cloud storage SDKs (@aws-sdk/client-s3, @aws-sdk/lib-storage, @azure/storage-blob, @google-cloud/storage) are now optional peer dependencies. Users only need to install the SDK(s) for the cloud provider they actually use. If a required SDK is missing at runtime, a clear, actionable error message is shown with the exact npm install command to fix it.

Changes

  • package.json: Moved cloud storage SDKs from devDependencies to peerDependencies and added peerDependenciesMeta marking all four SDKs as optional: true. Also added axios as a direct dependency.
  • srv/attachments/aws-s3.js: Wrapped require calls for @aws-sdk/client-s3 and @aws-sdk/lib-storage in a try/catch block to throw a helpful error if either SDK is missing.
  • srv/attachments/azure-blob-storage.js: Wrapped require for @azure/storage-blob in a try/catch to provide a clear install instruction on MODULE_NOT_FOUND.
  • srv/attachments/gcp.js: Wrapped require for @google-cloud/storage in a try/catch with an informative error message if the SDK is absent.
  • lib/mtx/server.js: Applied the same lazy-require pattern with helpful error messages for the cleanup functions targeting AWS S3, Azure Blob, and Google Cloud Storage.
  • tests/unit/optionalDeps.test.js: Added new unit tests verifying that each storage provider module throws a helpful error with the correct install command when its SDK is missing, and loads successfully when the SDK is present.
  • CHANGELOG.md: Documented this as a breaking change in the upcoming v4.0.0 release.

⚠️ Breaking Change: Projects that relied on these SDKs being bundled must now explicitly install the relevant cloud storage SDK(s).

  • 🔄 Regenerate and Update Summary
PR Bot Information

Version: 1.20.51

  • Output Template: Default Template
  • Summary Prompt: Default Prompt
  • Correlation ID: ca32d7d4-42b1-4341-9c88-0392b574989f
  • LLM: anthropic--claude-4.6-sonnet
  • Event Trigger: pull_request.opened
  • File Content Strategy: Full file content

@KoblerS KoblerS requested a review from a team as a code owner May 26, 2026 07:28
@KoblerS

KoblerS commented May 26, 2026

Copy link
Copy Markdown
Member Author

Closes #448

@hyperspace-insights hyperspace-insights Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The remaining tests (lines 41–74) have the same jest.isolateModules() return-value issue and should be fixed identically by adding return before each jest.isolateModules(...) call.


Summary: The PR correctly moves cloud storage SDKs to optional peer dependencies and adds runtime MODULE_NOT_FOUND guards with actionable error messages — the overall approach is sound. However, all seven tests in optionalDeps.test.js discard the return value of jest.isolateModules(), which means assertion failures inside those callbacks may not always be reliably propagated to Jest, making the test suite fragile. Each test should return jest.isolateModules(...) to ensure proper error propagation.

PR Bot Information

Version: 1.20.51

  • Correlation ID: ca32d7d4-42b1-4341-9c88-0392b574989f
  • Event Trigger: pull_request.opened
  • File Content Strategy: Full file content
  • LLM: anthropic--claude-4.6-sonnet

Comment thread tests/unit/optionalDeps.test.js
Comment thread tests/unit/optionalDeps.test.js
Comment thread tests/unit/optionalDeps.test.js
@stefanrudi

Copy link
Copy Markdown
Contributor

Is there a reason why we still use axios? We should probably move away from it and use Node's native fetch API.

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.

2 participants