Skip to content

Add framework detection for local builds#10444

Draft
falahat wants to merge 2 commits intouniversal_makerfrom
micro-microdiscovery
Draft

Add framework detection for local builds#10444
falahat wants to merge 2 commits intouniversal_makerfrom
micro-microdiscovery

Conversation

@falahat
Copy link
Copy Markdown
Contributor

@falahat falahat commented May 1, 2026

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

  • Test nextjs app local build
  • Test angular app local build
  • Test an app that has neither dependency in its package.json

We want to limit local builds to just nextjs to start.

We use their dependencies to detect if they're using angular or nextjs.
@falahat falahat changed the base branch from main to universal_maker May 1, 2026 16:43
Copy link
Copy Markdown
Contributor

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

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 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)

high

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)

medium

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)

medium

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.

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