Conversation
…irestore collections > can be bound to a getter
📝 WalkthroughWalkthroughUpdates CI workflow to set up Java 21 before the install phase, bumps firebase-functions from version 6 to 7 in functions and nuxt packages, expands nuxt peerDependencies to support firebase-functions 7.0.0, and refactors a collection test to use dynamic references instead of hardcoded names. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
.github/workflows/ci.ymlfunctions/package.jsonpackages/nuxt/package.jsontests/firestore/collection.spec.ts
🔇 Additional comments (4)
packages/nuxt/package.json (1)
49-49: LGTM! Dependency versions updated consistently.The firebase-functions upgrade is properly reflected in both peerDependencies (supporting v4-v7) and devDependencies (v7.0.0), maintaining backward compatibility while enabling development with the latest version.
Also applies to: 73-73
tests/firestore/collection.spec.ts (1)
405-427: Improved test clarity with explicit collection references.The refactored test is clearer and more maintainable:
- Explicitly creates
populatedCollectionRefandemptyCollectionRefinstead of relying on dynamic names- Tests the getter binding by switching
collectionRef.valuebetween the two references- Verifies that the bound collection updates from 2 items to 0 items
This approach makes the test's intent more obvious and easier to debug.
.github/workflows/ci.yml (1)
31-35: Java 21 is required by firebase-tools v14.19.0 and later.Firebase-tools v14.19.0 deprecated Java versions below 21 and now requires Java 21 for the emulator. This is a recent change; the emulator previously supported Java 11+. The CI configuration is correct.
functions/package.json (1)
18-18: Verify firebase-functions v7.0.0 compatibility and breaking changes in the codebase.Version 7.0.0 exists and is stable with no reported critical security advisories. However, significant breaking changes exist that must be addressed:
- Node.js runtime: Requires Node 18+ (drops Node 16 support)
- Configuration API:
functions.config()removed; migrate to params/secrets API (defineString,defineSecret,defineJsonSecret)- Function generation: Prefer 2nd-gen functions (v2) with updated imports from
firebase-functions/v2- Initialization: Review global state and cold-start assumptions for 2nd-gen lifecycle
Audit the codebase against the official v7 upgrade guide to confirm these breaking changes are properly addressed.
Tkkg1994
changed the title
|
Fixes #1646 |
commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1648 +/- ##
=======================================
Coverage 62.90% 62.90%
=======================================
Files 28 28
Lines 1903 1903
Branches 264 264
=======================================
Hits 1197 1197
Misses 706 706 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks a lot! Now you should be able to see a published version at https://github.com/vuejs/vuefire/actions/runs/20583068403/job/59115705184?pr=1648 and in a comment here I don't know when this will go forward, I still have to check with firebase and unfortunately, I currently have a lot of other pressing matters, so it will be probably months before I can merge this. Until that, you can use this PR's version |
|
Perfect, this looks really nice @posva Anything I can do to speed the process up? I have hoped to be able to install an official version soon. I can also be helpful to assist as I have a personal interest and maybe some more time than you 😄 Greetings Edit: and I would need the nuxt-module, so nuxt-vuefire |
|
Nothing you can do on your side, thanks! |
|
Thank you for the answer @posva As far as I can see I already included the correct changes here in 9f06992#diff-36b207d79434b922b863a3e8d4ca66d566cf0044d5f4d0943b208f7c19722dc2 So basically we should be able to build this without any further PR |
| }) | ||
|
|
||
| it('rejects on error', async () => { | ||
| // Collections don't naturally produce subscription errors in the emulator like documents do. |
There was a problem hiding this comment.
Ideally, we want to find a test replacement for this PR to get merged
There was a problem hiding this comment.
@posva any suggestion on how to implement this? In this case, I could help to also include this test with the latest 7.xx functions
|
Can we please merge this pr? It would really help since i can only install packages that are autorized by my organization and they only do that with official releases |
|
Dear @posva I truly want to help. Since I can't yet install this module (including the nuxt-version of it), what can I do to support a faster realease. Let me know, I will be here to help! Greetings 😄 |
|
@Tkkg1994 I'm currently busy with other work. I can make an exception and prioritize this work like I have done before if I get paid for it but since I disabled cal.com, sponsor $250 me one time instead and I will merge and release this |
3 participants
Summary by CodeRabbit
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.