Skip to content

Run Nx + Angular builds from root context #177

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
May 9, 2024
Merged

Run Nx + Angular builds from root context #177

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f26f57f
run monorepo builds from root context
blidd-google May 7, 2024
290ee3b
update gitignore
blidd-google May 7, 2024
2aa4116
add 'run' back into 'npm run build' and read build args from env
blidd-google May 8, 2024
d76cbb9
Merge branch 'main' into blidd-nx-angular-fixes
blidd-google May 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
/packages/**/*/dist/
/examples/
/dist
/packages/@apphosting/publish-dev/.local
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might be leftover from when you tested things locally?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this in case anyone wanted to use the local adapter publishing workflow, which will generate this directory to store downloaded npm modules.

/packages/@apphosting/publish-dev/htpasswd

# NPM
node_modules
Expand Down
26 changes: 16 additions & 10 deletions packages/@apphosting/adapter-angular/src/bin/build.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,22 +10,28 @@ import {

const root = process.cwd();

// Determine root of project to build
// Determine which build runner to use
let cmd = process.env.MONOREPO_COMMAND || DEFAULT_COMMAND;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let should be const instead, also could this line be moved to be closer to where cmd is used? Maybe line 26

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we fetch all of the Monorepo related env vars all at once maybe in some function and have it all documented there? I'm having a bit of a hard time following all of the different monorepo related env vars.

Also it might be nice to have the Monorepo related code extracted out to a new file in general with their own set of unit tests too especially if this may get more complex as time goes on. Doesn't have to be in the pr, but might be good to add a TODO

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this is absolutely something that should be done. Unfortunately there's a non-trivial amount of work to refactor this code into another module that can be imported by all adapters. In the interest of getting these fixes out in time for I/O I haven't addressed it in this PR, but it's at the top of my TODO list for sure


// Read environment variable (only relevant for monorepos with multiple targets)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you note what target means in this case in the comment too, I think it seems to indicate if the project is a monorepo or not, but that's not entirely clear

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep - I've also renamed the env var to "project" for more clarity

let target = process.env.MONOREPO_PROJECT || "";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const instead of let


// Determine root of project to build.
let projectRoot = root;
if (process.env.MONOREPO_PROJECT && process.env.FIREBASE_APP_DIRECTORY) {
// N.B. We don't want to change directories for monorepo builds, so that the build process can
// locate necessary files outside the project directory (e.g. at the root).
if (process.env.FIREBASE_APP_DIRECTORY && !target) {
projectRoot = projectRoot.concat("/", process.env.FIREBASE_APP_DIRECTORY);
const builder = process.env.MONOREPO_BUILDER || "";
checkMonorepoBuildConditions(builder);
}

// Check build conditions, which vary depending on your project structure (standalone or monorepo)
if (target) {
checkMonorepoBuildConditions(cmd, target);
} else {
await checkStandaloneBuildConditions(projectRoot);
}
// Determine which build runner to use
let cmd = DEFAULT_COMMAND;
if (process.env.MONOREPO_COMMAND) {
cmd = process.env.MONOREPO_COMMAND;
}

const outputBundleOptions = await build(projectRoot, cmd);
const outputBundleOptions = await build(projectRoot, cmd, target);
await generateOutputDirectory(root, outputBundleOptions);

await validateOutputDirectory(outputBundleOptions);
13 changes: 10 additions & 3 deletions packages/@apphosting/adapter-angular/src/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import fsExtra from "fs-extra";
import logger from "firebase-functions/logger";

import { fileURLToPath } from "url";
import { spawn } from "child_process";
import { spawn, execSync } from "child_process";
import { resolve, normalize, relative, dirname, join } from "path";
import { stringify as yamlStringify } from "yaml";
import {
Expand Down Expand Up @@ -64,7 +64,13 @@ export async function checkStandaloneBuildConditions(cwd: string): Promise<void>
/**
* Check if the monorepo build system is using the Angular application builder.
*/
export function checkMonorepoBuildConditions(builder: string): void {
export function checkMonorepoBuildConditions(cmd: string, target: string) {
let builder;
if (cmd === "nx") {
const output = execSync(`npx nx show project ${target}`);
const projectJson = JSON.parse(output.toString());
builder = projectJson.targets.build.executor;
}
if (builder !== REQUIRED_BUILDER) {
throw new Error(
"Only the Angular application builder is supported. Please refer to https://angular.dev/tools/cli/esbuild#for-existing-applications guide to upgrade your builder to the Angular application builder. ",
Expand Down Expand Up @@ -100,11 +106,12 @@ export function populateOutputBundleOptions(outputPaths: OutputPaths): OutputBun
export const build = (
projectRoot = process.cwd(),
cmd = DEFAULT_COMMAND,
...argv: string[]
): Promise<OutputBundleOptions> =>
new Promise((resolve, reject) => {
// enable JSON build logs for application builder
process.env.NG_BUILD_LOGS_JSON = "1";
const childProcess = spawn(cmd, ["run", "build"], {
const childProcess = spawn(cmd, ["build", ...argv], {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if cmd = DEFAULT_COMMAND then this line will run npm build. Is that a valid command? I think you might run into this error Unknown command: "build"

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah good catch - I thought npm build was still a valid alias for npm run build but not anymore. Will adjust accordingly

cwd: projectRoot,
shell: true,
stdio: ["inherit", "pipe", "pipe"],
Expand Down