Add firebase-functions v7 CI leg; make mockConfig() throw on v7+ - #334
Add firebase-functions v7 CI leg; make mockConfig() throw on v7+#334cabljac wants to merge 5 commits into
Conversation
functions.config() was removed in firebase-functions v7, so mockConfig() silently succeeding there is a trap: the mock "works" but every subsequent config() read throws. mockConfig() now detects the installed firebase-functions major version and throws with migration guidance on v7+, which makes the >=4.9.0 peer range honest. The unit CI job gains a firebase-functions matrix dimension: the existing pinned leg, plus a v7 leg that compiles the specs against the pinned typings, swaps in firebase-functions@^7, and runs mocha on the compiled output (the specs do not compile against v7 typings). The mockConfig specs are version-aware and assert the throw on v7. Refs #333
There was a problem hiding this comment.
Code Review
This pull request adds support for detecting the major version of firebase-functions to handle the removal of functions.config() in v7+, causing mockConfig() to throw an error with migration guidance on newer versions. The feedback suggests making the version parsing more robust by using parseInt to prevent NaN results, and exporting a unified _isConfigRemoved() helper to ensure consistent detection logic across both the source code and the test suites.
- export _isConfigRemoved() and use it in both spec files instead of branching on _firebaseFunctionsMajorVersion() - add defensive typeof check on config before probing - parse major version with parseInt and reject NaN instead of returning it - clear and restore K_CONFIGURATION around the feature-detect fallback so v4-v6 is not misclassified under GCFv2 - add CHANGELOG entry for the mockConfig() v7+ migration error - run lint/format only on the pinned CI matrix legs
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces changes to handle the removal of functions.config() in firebase-functions v7+. It adds detection logic to check the installed major version of firebase-functions and updates mockConfig() to throw a clear migration error when v7+ is detected. Test suites have been updated accordingly. The review feedback suggests caching the result of the _isConfigRemoved() check to avoid redundant file I/O and environment variable manipulation on subsequent calls.
Description
Refs #333 - implements the first two parts of the plan discussed there (the
latestcanary job is a follow-up and is intentionally not in this PR, so this does not close the issue).1.
mockConfig()now throws on firebase-functions v7+ (behavior change)functions.config()was removed in firebase-functions v7 (it throws at runtime and is typednever). PreviouslymockConfig()would silently "succeed" on a v7 install while every subsequentconfig()read threw - a confusing trap.mockConfig()now detects the installed firebase-functions major version and fails at the mock site with migration guidance:Detection resolves the installed
firebase-functionsentry point and walks up to itspackage.jsonto read the version (the package's exports map does not expose./package.json, on v4 or v7, so a plainrequire('firebase-functions/package.json')does not work). If the version cannot be determined, it falls back to feature-detectingconfig()on the v1 entry point.Behavior on v4-v6 installs is unchanged.
2. CI matrix: unit tests run against both pinned firebase-functions and v7
The unit job gains a
firebase-functions: [pinned, '7']matrix dimension. Thepinnedleg is exactly today's flow. The7leg compiles the specs against the pinned devDependency, swaps infirebase-functions@^7withnpm install --no-save, and runs mocha on the already-compiled output. Compile-then-swap is needed because the specs do not compile against v6/v7 typings (nested@types/expresslib-check conflict, plusconfigtypednever), and it mirrors how users actually hit version drift (#310): their compiled code running against a newer firebase-functions.The
#mockConfigspecs are version-aware: on the pinned leg they assert the existing mocking behavior; on the v7 leg they assert the new error (and thatCLOUD_RUNTIME_CONFIGis left untouched). The two lifecycle env-restore tests seedCLOUD_RUNTIME_CONFIGdirectly on v7+ since their subject iscleanup(), notmockConfig(). Nothing is skipped on either leg.Both legs are meant to be required checks - they target pinned majors, not floating
latest, so they cannot go red from an unrelated upstream release. A scheduled non-blocking canary againstfirebase-functions@latestis the remaining part of #333.Verified locally on Node 22:
npm ci && npm test- 119 passing, 0 failingnpx tsc,npm install --no-save firebase-functions@^7,npx mocha .tmp/spec/index.spec.js- 119 passing, 0 failing (v7 mockConfig assertions confirmed running), lint cleanCode sample