Fix Firebase Storage injected bucket loading - #110
Conversation
🦋 Changeset detectedLatest commit: 44a6c9a 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughFirebase Storage now loads Firebase Admin modules dynamically when needed. An already-initialized bucket bypasses Firebase Admin loading, with updated mocks and coverage for this behavior. ChangesLazy Firebase Admin loading
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant firebaseStorage
participant firebaseAdminLoader
participant FirebaseAdmin
alt Injected bucket
firebaseStorage->>firebaseStorage: Use provided Bucket
else Build bucket
firebaseStorage->>firebaseAdminLoader: Load Firebase Admin APIs
firebaseAdminLoader->>FirebaseAdmin: Require app or storage module
FirebaseAdmin-->>firebaseAdminLoader: Return module APIs
firebaseAdminLoader-->>firebaseStorage: Provide typed APIs
end
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
nice fix — pushed a small addition (44a6c9a) that adds the firebase-storage bundle to the build-output guard in |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@packages/files-sdk/test/build-output.test.ts`:
- Around line 156-157: Update ensureBuilt() to also verify the firebase-storage
bundle at dist/firebase-storage/index.js before skipping the build. Keep the
existing cliBundle and loaderBundle checks, and ensure the test’s firebaseBundle
path matches the artifact required by the readiness check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 22f9b299-889e-4610-b83d-c8f03ab378ff
📒 Files selected for processing (1)
packages/files-sdk/test/build-output.test.ts
|
thanks! agentpond's firebase package is now directly using files sdk: https://github.com/marcusschiesser/agentpond/releases/tag/agentpond%400.10.0 |
What changed
Bucketis suppliedApp, credential, and environment initialization behaviorWhy
firebaseStorage({ app: bucket })already accepts a pre-initialized Google Cloud Storage bucket, but the adapter's top-level Firebase Admin imports still requiredfirebase-adminto resolve before that bucket could be used. This prevented consumers that discover and initialize Firebase Admin themselves from injecting the resulting bucket without also making the peer resolvable from Files SDK's module location.Impact
This is a patch-level runtime fix with no public API changes. Firebase Admin remains an optional peer for every path that initializes or consumes a Firebase
App.Validation
bun test packages/files-sdk/test/firebase-storage.test.tsbun run types --filter files-sdkbun run checkbun run build --filter files-sdkSummary by CodeRabbit
Bug Fixes
Tests
firebase-storagebundle does not statically import optional peer dependencies, even across dynamic imports.