Repository navigation
feat: allow disabling automatic config discovery - #593
jabrailkhalil wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces an autoConfig option to skip the automatic discovery of default configuration files (superstatic.json and firebase.json). The review feedback identifies a bug in src/loaders/config-file.js where stringified JSON config objects are not correctly parsed and merged, resulting in dead code. Refactoring the parsing block and adding a unit test to verify merging behavior when autoConfig is enabled are recommended to resolve these issues.
| } catch { | ||
| if (isPlainObject(filename)) { | ||
| configObject = filename; | ||
| filename = CONFIG_FILE; | ||
| filename = defaultFiles; | ||
| } | ||
| } |
There was a problem hiding this comment.
There is a bug in how stringified JSON config objects (e.g., '{\"public\": \"app\"}') are handled.
Currently, if a stringified JSON object is passed, JSON.parse(filename) succeeds, so configObject is populated. However, filename remains the original JSON string. Because it is a string, it bypasses the catch block, and later fails the isPlainObject(filename) check on line 58. As a result, the default config files are never loaded or merged, which contradicts the intended behavior described in the comments.
Furthermore, even if isPlainObject(filename) on line 58 were to evaluate to true, setting filename = defaultFiles (an array) at that point is too late because the Array.isArray(filename) resolution block has already executed, meaning the array would never be resolved to an actual file path.
To fix both issues, we should set filename = defaultFiles immediately when a stringified JSON object is successfully parsed in the try block.
Here is the recommended refactoring for the parsing block:
try {
configObject = JSON.parse(filename);
if (isPlainObject(configObject)) {
filename = defaultFiles;
}
} catch {
if (isPlainObject(filename)) {
configObject = filename;
filename = defaultFiles;
}
}This ensures that both plain objects and stringified JSON objects are correctly identified and set to defaultFiles before the array resolution logic runs.
There was a problem hiding this comment.
I normalized parsed JSON objects before filename resolution in 5edcfd0. They now use the same default-file merge path as object configs, while autoConfig: false still bypasses discovery. Supplied values override defaults in both forms.
| // Set back to default config file if stringified object is | ||
| // given as config. With this, we override values in the config file | ||
| if (isPlainObject(filename)) { | ||
| filename = CONFIG_FILE; | ||
| filename = defaultFiles; | ||
| } |
There was a problem hiding this comment.
This block is dead code and can be safely removed once the parsing logic in the try block is updated to handle stringified objects.
As explained in the other comment, filename can never be a plain object at this point because any plain object or array would have already been resolved to a string or undefined by the preceding Array.isArray(filename) block.
There was a problem hiding this comment.
I removed the late fallback in 5edcfd0. Object inputs are now normalized before resolving the filename list, so the merge happens in one place.
| it("still loads a stringified config object", () => { | ||
| expect(loadConfigFile(JSON.stringify({ public: "app" }), false)).to.eql({ | ||
| public: "app", | ||
| }); | ||
| }); |
There was a problem hiding this comment.
To ensure that stringified JSON config objects are correctly merged with the default config files when autoConfig is enabled, we should add a test case for that scenario. Currently, there is no test verifying this behavior, and due to the bug in src/loaders/config-file.js where isPlainObject(filename) is checked instead of isPlainObject(configObject), this merging is silently failing.
it("still loads a stringified config object", () => {
expect(loadConfigFile(JSON.stringify({ public: "app" }), false)).to.eql({
public: "app",
});
});
it("merges a stringified config object with default config files when autoConfig is true", async () => {
await fs.writeFile(
"superstatic.json",
JSON.stringify({ cleanUrls: true }),
);
try {
expect(loadConfigFile(JSON.stringify({ public: "app" }), true)).to.eql({
public: "app",
cleanUrls: true,
});
} finally {
await fs.rm(
"superstatic.json",
{ force: true },
);
}
});There was a problem hiding this comment.
I added four failing-before/passing-after cases covering both default filenames with autoConfig omitted or explicitly true, plus coverage for JSON strings with discovery disabled. All 18 loader tests pass, and the full Linux suite passes with 247 tests and 15 pending. Build and lint also pass.
Fixes #503. Related: firebase/firebase-tools#9773.
Programmatic callers that already resolved their configuration can inherit unrelated headers or rewrites from
superstatic.jsonorfirebase.jsonin the working directory. I addedautoConfig: falseto skip that automatic discovery while keeping the supplied configuration. The default remainstrue, and explicitly named config files still load.Both ordinary configuration objects and JSON-encoded objects now follow the same discovery and override rules. The JSON-string path previously skipped default-file merging; it is now normalized before filename resolution, replacing the ineffective late fallback.
The option is available to middleware and server callers, with TypeScript types and README documentation. Firebase CLI would need to adopt it separately.
Validation:
npm run buildand fullnpm testpass on Linux with Node 22.23.3: 247 passing, 15 pending; lint reports no errors. The full run includes the HTTP configuration regressions.