Repository navigation
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds support for deploying applications located within Nx or Turborepo monorepos by building them from the monorepo's root directory. It introduces a buildDirectory utility to locate the monorepo root and configures Cloud Build with the GOOGLE_BUILDABLE environment variable. Feedback on the changes suggests marking the static MONOREPO_CONFIG_FILES array as as const to align with the repository's style guide. Additionally, the reviewer recommends normalizing path separators in the new unit tests to prevent failures on Windows environments.
| const NO_IMAGE = "scratch"; | ||
|
|
||
| /** Files that mark the root of an Nx or Turborepo monorepo, the monorepos App Hosting supports. */ | ||
| const MONOREPO_CONFIG_FILES = ["nx.json", "turbo.json"]; |
There was a problem hiding this comment.
According to the repository style guide, static configuration arrays and lookup tables should be marked as readonly or as const to prevent accidental modification and improve type safety.
| const MONOREPO_CONFIG_FILES = ["nx.json", "turbo.json"]; | |
| const MONOREPO_CONFIG_FILES = ["nx.json", "turbo.json"] as const; |
References
- Mark static configuration arrays and lookup tables as readonly or as const. (link)
| it("builds an app in an Nx or Turborepo monorepo from the monorepo's root", async () => { | ||
| sinon.stub(fs, "existsSync").callsFake((file) => file === "/project/turbo.json"); | ||
|
|
||
| await deployOne({ config: web }); | ||
|
|
||
| // The upload has the monorepo's root files, like its lockfile and shared packages. | ||
| expect(createArchiveStub).to.have.been.calledWithMatch({ backendId: "web" }, "/project"); | ||
| const { environmentVariables } = submitBuildStub.firstCall.args[2].buildpackBuild; | ||
| expect(environmentVariables).to.include({ GOOGLE_BUILDABLE: "apps/web" }); | ||
| }); |
There was a problem hiding this comment.
On Windows, path separators are backslashes (\\), which will cause the fs.existsSync stub comparison and the createArchiveStub assertion to fail because they use hardcoded forward-slash paths. Normalizing the paths by replacing backslashes with forward slashes makes the test cross-platform and robust.
it("builds an app in an Nx or Turborepo monorepo from the monorepo's root", async () => {
sinon.stub(fs, "existsSync").callsFake((file) => String(file).replace(/\\/g, "/") === "/project/turbo.json");
await deployOne({ config: web });
// The upload has the monorepo's root files, like its lockfile and shared packages.
expect(createArchiveStub).to.have.been.calledWithMatch(
{ backendId: "web" },
sinon.match((val: string) => val.replace(/\\/g, "/") === "/project")
);
const { environmentVariables } = submitBuildStub.firstCall.args[2].buildpackBuild;
expect(environmentVariables).to.include({ GOOGLE_BUILDABLE: "apps/web" });
});| describe("buildDirectory", () => { | ||
| /** Pretends that only these files exist. */ | ||
| function withFiles(...files: string[]): void { | ||
| sinon.stub(fs, "existsSync").callsFake((file) => files.includes(file as string)); | ||
| } | ||
|
|
||
| it("builds an app from its own folder if it isn't in a monorepo", () => { | ||
| withFiles("/project/apps/web/package.json"); | ||
| expect(buildDirectory("/project", "apps/web")).to.deep.equal({ | ||
| dir: "/project/apps/web", | ||
| app: "", | ||
| }); | ||
| expect(buildDirectory("/project")).to.deep.equal({ dir: "/project", app: "" }); | ||
| }); | ||
|
|
||
| it("builds an app in a monorepo from the monorepo's root", () => { | ||
| withFiles("/project/turbo.json"); | ||
| expect(buildDirectory("/project", "apps/web")).to.deep.equal({ | ||
| dir: "/project", | ||
| app: "apps/web", | ||
| }); | ||
| }); | ||
|
|
||
| it("uses the closest folder above the app with nx.json or turbo.json", () => { | ||
| withFiles("/project/turbo.json", "/project/frontend/nx.json"); | ||
| expect(buildDirectory("/project", "frontend/apps/web")).to.deep.equal({ | ||
| dir: "/project/frontend", | ||
| app: "apps/web", | ||
| }); | ||
| }); | ||
|
|
||
| it("builds from the app's folder if the app is the monorepo's root", () => { | ||
| withFiles("/project/apps/web/nx.json"); | ||
| expect(buildDirectory("/project", "apps/web")).to.deep.equal({ | ||
| dir: "/project/apps/web", | ||
| app: "", | ||
| }); | ||
| }); | ||
|
|
||
| it("doesn't look for a monorepo outside the project", () => { | ||
| withFiles("/turbo.json"); | ||
| expect(buildDirectory("/project", "apps/web")).to.deep.equal({ | ||
| dir: "/project/apps/web", | ||
| app: "", | ||
| }); | ||
| expect(buildDirectory("/project", "../other")).to.deep.equal({ dir: "/other", app: "" }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
To ensure the buildDirectory tests run successfully on Windows, we should normalize the path separators in both the withFiles stub helper and the test assertions. This avoids failures caused by the mismatch between Windows backslashes (\\) and hardcoded forward-slash paths.
describe("buildDirectory", () => {
/** Pretends that only these files exist. */
function withFiles(...files: string[]): void {
sinon.stub(fs, "existsSync").callsFake((file) => files.includes(String(file).replace(/\\/g, "/")));
}
function normalizeDir(res: { dir: string; app: string }) {
return { ...res, dir: res.dir.replace(/\\/g, "/") };
}
it("builds an app from its own folder if it isn't in a monorepo", () => {
withFiles("/project/apps/web/package.json");
expect(normalizeDir(buildDirectory("/project", "apps/web"))).to.deep.equal({
dir: "/project/apps/web",
app: "",
});
expect(normalizeDir(buildDirectory("/project"))).to.deep.equal({ dir: "/project", app: "" });
});
it("builds an app in a monorepo from the monorepo's root", () => {
withFiles("/project/turbo.json");
expect(normalizeDir(buildDirectory("/project", "apps/web"))).to.deep.equal({
dir: "/project",
app: "apps/web",
});
});
it("uses the closest folder above the app with nx.json or turbo.json", () => {
withFiles("/project/turbo.json", "/project/frontend/nx.json");
expect(normalizeDir(buildDirectory("/project", "frontend/apps/web"))).to.deep.equal({
dir: "/project/frontend",
app: "apps/web",
});
});
it("builds from the app's folder if the app is the monorepo's root", () => {
withFiles("/project/apps/web/nx.json");
expect(normalizeDir(buildDirectory("/project", "apps/web"))).to.deep.equal({
dir: "/project/apps/web",
app: "",
});
});
it("doesn't look for a monorepo outside the project", () => {
withFiles("/turbo.json");
expect(normalizeDir(buildDirectory("/project", "apps/web"))).to.deep.equal({
dir: "/project/apps/web",
app: "",
});
expect(normalizeDir(buildDirectory("/project", "../other"))).to.deep.equal({ dir: "/other", app: "" });
});
});feebf33 to
d570892
Compare
…loys, like App Hosting's preparer
d570892 to
947cf86
Compare
Description
Follow-up to Wei's comment on #11205. For builds on Cloud Build,
firebase deploy --only runzipped only the service'srootDir. So an app in a monorepo was built without the monorepo's root lockfile and shared packages.This makes builds on Cloud Build work like App Hosting's:
.gitignoreand the service'signorelist still apply.GOOGLE_BUILDABLEtorootDirwhen it's a subfolder, so the buildpacks know which folder is the app.The
GOOGLE_BUILDABLErule comes from App Hosting's local builds. It's now one exported helper,googleBuildableinapphosting/localbuilds.ts, used by App Hosting's local builds, Cloud Run's local builds, and Cloud Run's builds on Cloud Build. App Hosting's behavior doesn't change.Cloud Run's local builds already built from the whole project with this rule, because they reuse App Hosting's local build code.
Scenarios Tested
GOOGLE_BUILDABLEtests inlocalbuilds.spec.ts, which now run through the helper.Sample Commands