Keep @aws-sdk out of the static graph behind files-sdk/r2 - #109
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 68c9eb4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughThe S3 adapter implementation moves into an injectable core. R2 dynamically loads the core and AWS SDK modules on first use, while S3 retains static wiring. Build-output tests now traverse dynamic imports to verify optional AWS peers remain absent from the R2 bundle. ChangesR2 lazy AWS SDK loading
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant R2
participant S3Core
participant AWSSDK
R2->>AWSSDK: Dynamically import S3 and presigning modules
R2->>S3Core: Call createS3Adapter with loaded modules
S3Core->>AWSSDK: Construct client and execute S3 operations
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/files-sdk/src/s3/core.ts (1)
136-145: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve the underlying import error as
cause.A bare
catchturns any module-initialization failure inside@aws-sdk/lib-storageinto a misleading "install the optional peer" message. Keeping the original error makes the rare non-ERR_MODULE_NOT_FOUNDcase debuggable.♻️ Proposed change
const loadLibStorage = async () => { try { return await import("`@aws-sdk/lib-storage`"); - } catch { + } catch (error) { throw new FilesError( "Provider", - "Multipart and progress uploads on S3 require the optional peer dependency '`@aws-sdk/lib-storage`'. Install it to use the `multipart` or `onProgress` options." + "Multipart and progress uploads on S3 require the optional peer dependency '`@aws-sdk/lib-storage`'. Install it to use the `multipart` or `onProgress` options.", + error ); } };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/files-sdk/src/s3/core.ts` around lines 136 - 145, Update loadLibStorage to capture the import exception and pass it as the FilesError cause, while retaining the existing user-facing message. Preserve the original error for all module-loading failures, including optional-dependency and initialization errors.packages/files-sdk/test/build-output.test.ts (1)
137-147: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard
r2/index.jsinensureBuilt(). The build emitsdist/r2/index.js, butensureBuilt()only gates oncliBundleandloaderBundle, so a staledist/can still reachreadFileSyncwith an ENOENT here. Addr2to the precondition or check the file explicitly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/files-sdk/test/build-output.test.ts` around lines 137 - 147, Update ensureBuilt() to also verify that the generated r2/index.js bundle exists before tests read it, either by including the r2 artifact in its existing build precondition or by explicitly checking that file. Preserve the existing cliBundle and loaderBundle checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/files-sdk/src/s3/core.ts`:
- Around line 136-145: Update loadLibStorage to capture the import exception and
pass it as the FilesError cause, while retaining the existing user-facing
message. Preserve the original error for all module-loading failures, including
optional-dependency and initialization errors.
In `@packages/files-sdk/test/build-output.test.ts`:
- Around line 137-147: Update ensureBuilt() to also verify that the generated
r2/index.js bundle exists before tests read it, either by including the r2
artifact in its existing build precondition or by explicitly checking that file.
Preserve the existing cliBundle and loaderBundle checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 908b8fa7-f2f0-4976-b43c-ee72c0ff8332
📒 Files selected for processing (5)
.changeset/r2-lazy-aws-sdk.mdpackages/files-sdk/src/r2/index.tspackages/files-sdk/src/s3/core.tspackages/files-sdk/src/s3/index.tspackages/files-sdk/test/build-output.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #105.
Problem
Building a Worker that uses
files-sdk/r2in binding (or fetch) mode failed under rolldown-vite with oneMISSING_EXPORTper@aws-sdk/client-s3named import when the optional@aws-sdk/*peers weren't installed. The aws-sdk HTTP engine is lazy at runtime (await import("../s3/index.js")), but consumer bundlers resolve dynamically-reached chunks at build time too — and the s3 entry's static named imports then hard-error against Vite's optional-peer-dep placeholder, which ismodule.exports = {}plus a throw (no named exports).Fix
No static
@aws-sdk/*value import remains anywhere in the module graph reachable fromfiles-sdk/r2:src/s3/core.ts(moved froms3/index.ts, so the diff is mostly the rename): the engine is now parameterized over anS3Sdkbundle of the three SDK modules, importing only their types.src/s3/index.ts: thin static wiring — same named imports as before (tree-shakeable), same syncs3()factory andrawclient. No behavior change forfiles-sdk/s3consumers or the s3-compatible wrappers.src/r2/index.ts:lazyS3now dynamically imports../s3/core.jsplus the three@aws-sdk/*modules. Dynamic specifiers only ever resolve to the placeholder as a chunk that never executes on the binding/fetch paths, so bundling succeeds.Tests
build-output.test.ts: walks the dist r2 graph the way a bundler does (through dynamic imports) and asserts optional peers appear only as dynamic specifiers. The walk flags the pre-fix layout and the s3 entry (which legitimately imports the SDK statically), so it demonstrably bites.s3/core.ts98.41/98.80).r2({...}).url()presigns offline through the newPromise.allload,rawreflects the loadedS3Client, and the entire lazily-reachable chunk graph contains no static@aws-sdkimport.Summary by CodeRabbit
New Features
Bug Fixes