Skip to content

Add warning logs for Local Builds + ABIU if using the wrong versions of nodejs#10698

Draft
falahat wants to merge 6 commits into
mainfrom
local_build_fix_abiu
Draft

Add warning logs for Local Builds + ABIU if using the wrong versions of nodejs#10698
falahat wants to merge 6 commits into
mainfrom
local_build_fix_abiu

Conversation

@falahat

@falahat falahat commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

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

…of nodejs which conflict with ABIU's version
@wiz-9635d3485b

wiz-9635d3485b Bot commented Jun 22, 2026

Copy link
Copy Markdown

Wiz Scan Summary

Scanner Findings
Vulnerability Finding Vulnerabilities -
Data Finding Sensitive Data -
Secret Finding Secrets -
IaC Misconfiguration IaC Misconfigurations -
SAST Finding SAST Findings 4 Medium
Software Management Finding Software Management Findings -
Total 4 Medium

View scan details in Wiz

To detect these findings earlier in the dev lifecycle, try using Wiz Code VS Code Extension.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/apphosting/localbuilds.ts Outdated
Comment thread src/apphosting/localbuilds.spec.ts
@falahat

falahat commented Jun 23, 2026

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/deploy/apphosting/prepare.ts Outdated
Comment on lines +191 to +194
const backend = backends.find((b) => parseBackendName(b.name).id === cfg.backendId);
if (!backend) {
throw new FirebaseError(`Backend ${cfg.backendId} not found.`);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

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.`);
    }

Comment thread src/apphosting/localbuilds.ts Outdated
falahat and others added 3 commits June 24, 2026 09:09
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Comment on lines +350 to +356
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.`,
);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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" },

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't modify the other test cases (aka, don't shove nodejs22 into unrelated tests here.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants