add flag to override host/port for the emulators#10761
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a --host-overrides flag for the emulators:start and emulators:exec commands, allowing users to dynamically override the host and port of specific emulators. It includes parsing logic for various host/port formats (including IPv6) and corresponding unit tests. The review feedback suggests enhancing the robustness of the parsing logic by adding defensive type checks, trimming whitespace from input tokens to prevent parsing failures, rejecting overrides for the extensions emulator, and validating that parsed ports fall within the valid TCP range (1 to 65535).
| if (options.hostOverrides) { | ||
| options.hostOverrides = parseHostOverrides(options.hostOverrides); | ||
| } |
There was a problem hiding this comment.
Add a defensive type check to ensure options.hostOverrides is a string before parsing it. If beforeEmulatorCommand is called programmatically or in tests where options.hostOverrides is already parsed into an object, calling parseHostOverrides on it again will throw a runtime error.
| if (options.hostOverrides) { | |
| options.hostOverrides = parseHostOverrides(options.hostOverrides); | |
| } | |
| if (typeof options.hostOverrides === "string") { | |
| options.hostOverrides = parseHostOverrides(options.hostOverrides); | |
| } |
References
- TypeScript: Use strict null checks and handle undefined/null explicitly. (link)
| const overrides = hostOverrides.split(","); | ||
| for (const override of overrides) { |
There was a problem hiding this comment.
Trim each override and filter out empty elements when splitting hostOverrides. This prevents parsing failures if the user includes spaces after commas (e.g., firestore:8080, auth:9099) or trailing commas.
| const overrides = hostOverrides.split(","); | |
| for (const override of overrides) { | |
| const overrides = hostOverrides.split(",").map((o) => o.trim()).filter(Boolean); | |
| for (const override of overrides) { |
| const emulatorName = override.substring(0, colonIndex); | ||
| if (!isEmulator(emulatorName)) { | ||
| throw new FirebaseError( | ||
| `Invalid --host-overrides value. "${emulatorName}" is not a valid emulator name. Valid emulators are: ${Object.values( | ||
| Emulators, | ||
| ).join(", ")}`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Trim the parsed emulatorName to prevent whitespace issues. Additionally, explicitly throw an error if the user tries to override the extensions emulator. Since extensions runs on the same port as functions and is excluded from getListenConfig, any override for extensions would be silently ignored. Directing the user to override functions instead prevents confusion.
const emulatorName = override.substring(0, colonIndex).trim();
if (!isEmulator(emulatorName)) {
throw new FirebaseError(
`Invalid --host-overrides value. "${emulatorName}" is not a valid emulator name. Valid emulators are: ${Object.values(
Emulators,
).join(", ")}`,
);
}
if (emulatorName === Emulators.EXTENSIONS) {
throw new FirebaseError(
`Invalid --host-overrides value. The "extensions" emulator runs on the same port as the "functions" emulator. Please override "functions" instead.`
);
}References
- Throw FirebaseError for expected, user-facing errors. (link)
| const hostOverride = override.substring(colonIndex + 1); | ||
| if (hostOverride.length === 0) { |
There was a problem hiding this comment.
Trim hostOverride to ensure that any leading or trailing whitespace around the host/port portion is ignored, preventing regex validation failures.
| const hostOverride = override.substring(colonIndex + 1); | |
| if (hostOverride.length === 0) { | |
| const hostOverride = override.substring(colonIndex + 1).trim(); | |
| if (hostOverride.length === 0) { |
| const [hostString, portString] = hostOverride.split(":"); | ||
| if (!/^\d+$/.test(portString)) { |
There was a problem hiding this comment.
Trim the split host and port strings to prevent validation failures if there are spaces around the colon (e.g., localhost : 8080).
| const [hostString, portString] = hostOverride.split(":"); | |
| if (!/^\d+$/.test(portString)) { | |
| const [hostString, portString] = hostOverride.split(":").map((s) => s.trim()); | |
| if (!/^\d+$/.test(portString)) { |
| if (isNaN(port)) { | ||
| throw new FirebaseError( | ||
| `Invalid --host-overrides value "${override}". Port must be a number.`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Validate that the parsed port is within the valid TCP port range (1 to 65535). Currently, ports like 0 or 999999 are accepted during parsing because they consist of digits, but they are not valid TCP ports. Validating the port range early prevents binding failures later on.
| if (isNaN(port)) { | |
| throw new FirebaseError( | |
| `Invalid --host-overrides value "${override}". Port must be a number.`, | |
| ); | |
| } | |
| if (isNaN(port) || port < 1 || port > 65535) { | |
| throw new FirebaseError( | |
| `Invalid --host-overrides value "${override}". Port must be a valid number between 1 and 65535.`, | |
| ); | |
| } |
References
- Throw FirebaseError for expected, user-facing errors. (link)
2 participants
Description
Proposed fix for #10756. Adding a flag for each emulator might make it difficult to maintain since we would have to add a flag whenever a new emulator is added, so opt for using a single flag to override host/port for the emulators. Usage:
firebase emulators:start --host-overrides "functions:[::1]:8082,auth:9098".Scenarios Tested
ipv6 address - works
ipv4 address - works
emulator:port - works
invalid format - correctly throws error
Sample Commands
firebase emulators:start --host-overrides functions:127.0.0.1:8082,auth:9098