Add warning logs for Local Builds + ABIU if using the wrong versions of nodejs#10698
Add warning logs for Local Builds + ABIU if using the wrong versions of nodejs#10698falahat wants to merge 6 commits into
Conversation
…of nodejs which conflict with ABIU's version
Wiz Scan Summary
To detect these findings earlier in the dev lifecycle, try using Wiz Code VS Code Extension. |
There was a problem hiding this comment.
Code Review
This pull request introduces a new validation function, validateLocalBuildNodeVersion, to ensure that the local Node.js environment and package.json configuration are compatible with the target backend's ABIU runtime settings. Feedback on the implementation highlights that using semver.satisfies with a hardcoded target version could lead to false positives for minor/patch constraints and potential crashes on invalid semver ranges; using semver.intersects within a try/catch block is recommended instead, along with an accompanying unit test.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a validation step (validateLocalBuildNodeVersion) during local builds to ensure that the local Node.js environment and package.json engine configuration are compatible with the target backend's ABIU runtime settings. Feedback on the changes highlights two critical issues: first, a potential crash in the deployment preparation step if a newly created backend is not found in the pre-fetched list, which can be resolved by dynamically fetching the backend; second, a potential TypeError crash in semver.intersects if the engines.node field contains an invalid semver range, which should be guarded using semver.validRange.
| const backend = backends.find((b) => parseBackendName(b.name).id === cfg.backendId); | ||
| if (!backend) { | ||
| throw new FirebaseError(`Backend ${cfg.backendId} not found.`); | ||
| } |
There was a problem hiding this comment.
If a backend is newly created during the prepare step (via doSetupSourceDeploy), it will not be present in the pre-fetched backends list. This will cause backends.find to return undefined and throw a FirebaseError, crashing the deployment process.
To fix this, dynamically fetch the backend details from the API if it is not found in the pre-fetched list.
let backend = backends.find((b) => parseBackendName(b.name).id === cfg.backendId);
if (!backend) {
const location = context.backendLocations[cfg.backendId];
if (location) {
const apphosting = await import("../../gcp/apphosting");
try {
backend = await apphosting.getBackend(projectId, location, cfg.backendId);
} catch {
// Fall through to error handling below
}
}
}
if (!backend) {
throw new FirebaseError(`Backend ${cfg.backendId} not found.`);
}Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…ools into local_build_fix_abiu
| logLabeledWarning( | ||
| "apphosting", | ||
| `Local Node.js version (${localNodeVersion}) does not match your backend's target ABIU runtime major version (Node.js ${targetMajor}). ` + | ||
| `Because local builds compile code using your host's Node.js version, this mismatch can cause native modules ` + | ||
| `to be compiled for the wrong version, leading to silent startup crashes in production (running on Node.js ${targetMajor}). ` + | ||
| `Please switch your local environment to Node.js ${targetMajor} to ensure build-to-run parity.`, | ||
| ); |
There was a problem hiding this comment.
less verbose, less specific warning. Just note that this may cause issues.
| { | ||
| name: "projects/my-project/locations/us-central1/backends/foo", | ||
| appId: "my-app-id", | ||
| runtime: { value: "nodejs22" }, |
There was a problem hiding this comment.
We shouldn't modify the other test cases (aka, don't shove nodejs22 into unrelated tests here.)
Description
Universal Maker will use the user's own nodejs version installed locally to build the app. It does NOT use package.json#engines nor backend.runtime (the ABIU setting.)
However, for the runtime nodejs version, we DO respect the ABIU setting. That means you can build an app locally with nodejs 22 and then deploy it, using nodejs24 during runtime. In fact, I tested this and it worked.
This PR adds warning logs to detect if you are using inconsistent nodejs versions. For the most part, we just log warnings so that you are aware if this doesnt work, but we don't block users (unless ABIU is not enabled at all.)
Scenarios Tested
Sample Commands