Optional deps#453
Conversation
|
Closes #448 |
![hyperspace-insights[bot]](https://avatars.githubusercontent.com/u/2531208?s=60&v=4){kind=small}
There was a problem hiding this comment.
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
| test("aws-s3 throws helpful error when @aws-sdk/client-s3 is missing", () => { | ||
| jest.isolateModules(() => { | ||
| jest.doMock("@aws-sdk/client-s3", () => { | ||
| throw makeNotFound("@aws-sdk/client-s3") | ||
| }) | ||
| expect(() => require("../../srv/attachments/aws-s3")).toThrow( | ||
| "npm install @aws-sdk/client-s3 @aws-sdk/lib-storage", | ||
| ) | ||
| }) |
There was a problem hiding this comment.
Bug: expect assertions inside jest.isolateModules() callbacks are silently swallowed on failure
jest.isolateModules() executes the callback synchronously and propagates thrown errors, but the callback's return value is discarded. If an expect(...).toThrow() assertion fails (i.e., no error is thrown), expect itself throws internally — and that will propagate, so failure detection works in that direction. However, for expect(...).not.toThrow() (lines 56, 65, 72), if the module does throw, the error is thrown inside the callback but Jest may not always reliably associate it with the enclosing test, especially when modules load side-effects or when an unhandled rejection occurs.
More critically, these tests that assert successful loading (.not.toThrow()) load real modules that have real side-effects (e.g., aws-s3.js calls cds.log(...), requires ../../lib/helper, etc.). If those side-effect requires fail, the error propagates, but the isolation is not guaranteed to be clean across tests because jest.dontMock inside jest.isolateModules doesn't reset the mock registry for modules that were previously mocked in earlier tests.
The safest pattern is to return the jest.isolateModules call from the test callback, ensuring the module registry is properly scoped:
| test("aws-s3 throws helpful error when @aws-sdk/client-s3 is missing", () => { | |
| jest.isolateModules(() => { | |
| jest.doMock("@aws-sdk/client-s3", () => { | |
| throw makeNotFound("@aws-sdk/client-s3") | |
| }) | |
| expect(() => require("../../srv/attachments/aws-s3")).toThrow( | |
| "npm install @aws-sdk/client-s3 @aws-sdk/lib-storage", | |
| ) | |
| }) | |
| test("aws-s3 throws helpful error when @aws-sdk/client-s3 is missing", () => { | |
| return jest.isolateModules(() => { | |
| jest.doMock("@aws-sdk/client-s3", () => { | |
| throw makeNotFound("@aws-sdk/client-s3") | |
| }) | |
| expect(() => require("../../srv/attachments/aws-s3")).toThrow( | |
| "npm install @aws-sdk/client-s3 @aws-sdk/lib-storage", | |
| ) | |
| }) | |
| }) | |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| test("aws-s3 throws helpful error when @aws-sdk/lib-storage is missing", () => { | ||
| jest.isolateModules(() => { | ||
| jest.doMock("@aws-sdk/lib-storage", () => { | ||
| throw makeNotFound("@aws-sdk/lib-storage") | ||
| }) | ||
| expect(() => require("../../srv/attachments/aws-s3")).toThrow( | ||
| "npm install @aws-sdk/client-s3 @aws-sdk/lib-storage", | ||
| ) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Bug: expect assertions inside jest.isolateModules() callbacks are silently swallowed on failure
Same issue as the first test — jest.isolateModules() return value is discarded. Return it to ensure the assertion result is propagated to Jest properly.
| test("aws-s3 throws helpful error when @aws-sdk/lib-storage is missing", () => { | |
| jest.isolateModules(() => { | |
| jest.doMock("@aws-sdk/lib-storage", () => { | |
| throw makeNotFound("@aws-sdk/lib-storage") | |
| }) | |
| expect(() => require("../../srv/attachments/aws-s3")).toThrow( | |
| "npm install @aws-sdk/client-s3 @aws-sdk/lib-storage", | |
| ) | |
| }) | |
| }) | |
| test("aws-s3 throws helpful error when @aws-sdk/lib-storage is missing", () => { | |
| return jest.isolateModules(() => { | |
| jest.doMock("@aws-sdk/lib-storage", () => { | |
| throw makeNotFound("@aws-sdk/lib-storage") | |
| }) | |
| expect(() => require("../../srv/attachments/aws-s3")).toThrow( | |
| "npm install @aws-sdk/client-s3 @aws-sdk/lib-storage", | |
| ) | |
| }) | |
| }) | |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| test("azure-blob-storage throws helpful error when @azure/storage-blob is missing", () => { | ||
| jest.isolateModules(() => { | ||
| jest.doMock("@azure/storage-blob", () => { | ||
| throw makeNotFound("@azure/storage-blob") | ||
| }) | ||
| expect(() => require("../../srv/attachments/azure-blob-storage")).toThrow( | ||
| "npm install @azure/storage-blob", | ||
| ) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Bug: jest.isolateModules() return value discarded — assertion failures may not propagate reliably
All remaining jest.isolateModules() calls discard the return value. Return it from the test callback so Jest can properly handle any thrown assertion errors.
| test("azure-blob-storage throws helpful error when @azure/storage-blob is missing", () => { | |
| jest.isolateModules(() => { | |
| jest.doMock("@azure/storage-blob", () => { | |
| throw makeNotFound("@azure/storage-blob") | |
| }) | |
| expect(() => require("../../srv/attachments/azure-blob-storage")).toThrow( | |
| "npm install @azure/storage-blob", | |
| ) | |
| }) | |
| }) | |
| test("azure-blob-storage throws helpful error when @azure/storage-blob is missing", () => { | |
| return jest.isolateModules(() => { | |
| jest.doMock("@azure/storage-blob", () => { | |
| throw makeNotFound("@azure/storage-blob") | |
| }) | |
| expect(() => require("../../srv/attachments/azure-blob-storage")).toThrow( | |
| "npm install @azure/storage-blob", | |
| ) | |
| }) | |
| }) | |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
… Azure, and GCP storage providers
…into optional-deps
1 participant
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 exactnpm installcommand to fix it.Changes
package.json: Moved cloud storage SDKs fromdevDependenciestopeerDependenciesand addedpeerDependenciesMetamarking all four SDKs asoptional: true. Also addedaxiosas a direct dependency.srv/attachments/aws-s3.js: Wrappedrequirecalls for@aws-sdk/client-s3and@aws-sdk/lib-storagein atry/catchblock to throw a helpful error if either SDK is missing.srv/attachments/azure-blob-storage.js: Wrappedrequirefor@azure/storage-blobin atry/catchto provide a clear install instruction onMODULE_NOT_FOUND.srv/attachments/gcp.js: Wrappedrequirefor@google-cloud/storagein atry/catchwith 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 upcomingv4.0.0release.PR Bot Information
Version:
1.20.51ca32d7d4-42b1-4341-9c88-0392b574989fanthropic--claude-4.6-sonnetpull_request.opened