Skip to content

Remove Local Build Universal Maker hack #10539

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
falahat wants to merge 6 commits into
base: local_build_fix_envvars
Choose a base branch
from
Draft

Remove Local Build Universal Maker hack #10539

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
dcc1e2a
get rid of Universal Maker hack and make the toProcessEnv method clea…
falahat May 20, 2026
eaf38bb
merge: local_build_fix_envvars formatting and JSDoc fixes
falahat May 20, 2026
8f5a50e
style: auto-format and lint fixes for local_build_remove_um_hack
falahat May 20, 2026
383a69c
style: fix TS linter warnings in localbuilds.ts
falahat May 20, 2026
530d7d5
style: resolve ESLint single-line formatting error in localbuilds.ts
falahat May 20, 2026
a589888
merge: local_build_fix_envvars readFileSync typesafety and sorting fixes
falahat May 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 16 additions & 40 deletions src/apphosting/localbuilds.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,9 @@ function executeUniversalMakerBinary(
addedEnv: Record<string, string> = {},
): void {
try {
const bundleOutput = path.join(projectRoot, "bundle_output");
fs.removeSync(bundleOutput);
fs.ensureDirSync(bundleOutput);
const targetAppHosting = path.join(projectRoot, ".apphosting");
fs.removeSync(targetAppHosting);
fs.ensureDirSync(targetAppHosting);

const res = childProcess.spawnSync(
universalMakerBinary,
Expand All @@ -55,7 +55,7 @@ function executeUniversalMakerBinary(
...process.env,
...addedEnv,
X_GOOGLE_TARGET_PLATFORM: "fah",
FIREBASE_OUTPUT_BUNDLE_DIR: bundleOutput,
FIREBASE_OUTPUT_BUNDLE_DIR: targetAppHosting,
},
stdio: "pipe",
},
Expand Down Expand Up @@ -126,24 +126,6 @@ function processUniversalMakerOutput(projectRoot: string): AppHostingBuildOutput
const outputRaw = fs.readFileSync(outputFilePath, "utf-8");
fs.unlinkSync(outputFilePath); // Clean up temporary metadata file

const bundleOutput = path.join(projectRoot, "bundle_output");
const targetAppHosting = path.join(projectRoot, ".apphosting");

// Universal Maker has a bug where it accidentally empties bundle.yaml if we tell it to output directly to .apphosting.
// To avoid this, we output to bundle_output first, and then safely move the files over.
if (fs.existsSync(bundleOutput)) {
fs.ensureDirSync(targetAppHosting);
const files = fs.readdirSync(bundleOutput);
for (const file of files) {
const dest = path.join(targetAppHosting, file);
if (fs.existsSync(dest)) {
fs.removeSync(dest);
}
fs.moveSync(path.join(bundleOutput, file), dest);
}
fs.removeSync(bundleOutput);
}

let umOutput: UniversalMakerOutput;
try {
umOutput = JSON.parse(outputRaw) as UniversalMakerOutput;
Expand Down Expand Up @@ -245,10 +227,7 @@ export async function localBuild(
}

const addedEnv = await toProcessEnv(projectId, env);
const apphostingBuildOutput = await runUniversalMaker(
projectRoot,
addedEnv as Record<string, string>,
);
const apphostingBuildOutput = await runUniversalMaker(projectRoot, addedEnv);

const annotations: Record<string, string> = Object.fromEntries(
Object.entries(apphostingBuildOutput.metadata).map(([key, value]) => [key, String(value)]),
Expand All @@ -273,22 +252,19 @@ export async function localBuild(
};
}

async function toProcessEnv(projectId: string, env: EnvMap): Promise<NodeJS.ProcessEnv> {
const entries = await Promise.all(
Object.entries(env).map(async ([key, value]) => {
if (value.availability && !value.availability.includes("BUILD")) {
return null;
}
async function toProcessEnv(projectId: string, env: EnvMap): Promise<Record<string, string>> {
const buildVars = Object.entries(env).filter(([, value]) => {
return !value.availability || value.availability.includes("BUILD");
});

if (value.secret) {
const resolvedValue = await loadSecret(projectId, value.secret);
return [key, resolvedValue];
} else {
return [key, value.value || ""];
}
const resolvedEntries = await Promise.all(
buildVars.map(async ([key, value]) => {
const resolvedValue = value.secret
? await loadSecret(projectId, value.secret)
: value.value || "";
return [key, resolvedValue];
}),
);

const filteredEntries = entries.filter((entry): entry is [string, string] => entry !== null);
return Object.fromEntries(filteredEntries) as NodeJS.ProcessEnv;
return Object.fromEntries(resolvedEntries) as Record<string, string>;
}
Comment on lines +255 to 270
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot May 20, 2026

Choose a reason for hiding this comment

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

medium

The refactoring of toProcessEnv significantly improves readability and adheres to the repository's best practice of reducing nesting. By filtering the environment variables before mapping them to asynchronous secret resolution, the code is more efficient and easier to follow. Additionally, changing the return type to Record<string, string> provides better type safety and aligns with the expectations of runUniversalMaker.

References
  1. Reduce nesting as much as possible. Code should avoid unnecessarily deep nesting or long periods of nesting. Handle edge cases early and exit or fold them into the general case.

Loading