Add framework detection for local builds#10444
Add framework detection for local builds#10444falahat wants to merge 2 commits intouniversal_makerfrom
Conversation
We want to limit local builds to just nextjs to start. We use their dependencies to detect if they're using angular or nextjs.
There was a problem hiding this comment.
Code Review
This pull request introduces an experimental feature for local App Hosting builds using a standalone "Universal Maker" binary. The changes include logic for platform-specific binary downloading and caching, execution of the build process, and parsing of generated metadata. It also adds framework detection to limit local builds to Next.js and centralizes file validation utilities. Review feedback highlights the need for better error handling in file streams to prevent CLI hangs, improved logging for malformed configuration files, and a recommendation to use asynchronous process spawning to keep the event loop responsive during long builds.
I am having trouble creating individual review comments. Click here to see my feedback.
src/downloadUtils.ts (77-92)
The validateChecksum function is missing error handling for the file read stream. If an error occurs while reading the file (e.g., due to permission changes or disk issues), the promise will remain pending indefinitely, causing the CLI to hang. You should add a listener for the error event on the stream to reject the promise.
return new Promise((resolve, reject) => {
const hash = crypto.createHash(algorithm);
const stream = fs.createReadStream(filepath);
stream.on("data", (data: Buffer | string) => hash.update(data));
stream.on("error", (err) => reject(err));
stream.on("end", () => {
const checksum = hash.digest("hex");
return checksum === expectedChecksum
? resolve()
: reject(
new FirebaseError(
`download failed, expected checksum ${expectedChecksum} but got ${checksum}`,
{ exit: 1 },
),
);
});
});src/apphosting/utils.ts (111-115)
Returning undefined silently when JSON.parse fails might make it difficult for users to diagnose issues with a malformed package.json. While returning undefined is acceptable for the detection logic, consider logging a debug message or a warning to inform the user that the file could not be parsed.
src/apphosting/localbuilds.ts (47-58)
Using childProcess.spawnSync blocks the Node.js event loop until the process completes. While this is often acceptable in CLI tools, for long-running build processes, it prevents any asynchronous tasks (like progress updates or heartbeats) from running. Consider using the asynchronous childProcess.spawn wrapped in a Promise to keep the event loop responsive.
Description
We only want to support nextjs to start. We also want to give the user a clear error message if they try to use angular
Scenarios Tested