Run Nx + Angular builds from root context #177
Conversation
blidd-google
requested review from
Yuangwang,
sjjj986,
jamesdaniels and
leoortizz
as code owners
blidd-google
force-pushed
the
blidd-nx-angular-fixes
branch
from
b7970b8 to
290ee3b
Compare
| @@ -1,6 +1,8 @@ | |||
| /packages/**/*/dist/ | |||
| /examples/ | |||
| /dist | |||
| /packages/@apphosting/publish-dev/.local | |||
There was a problem hiding this comment.
I think this might be leftover from when you tested things locally?
There was a problem hiding this comment.
I added this in case anyone wanted to use the local adapter publishing workflow, which will generate this directory to store downloaded npm modules.
| @@ -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; | |||
There was a problem hiding this comment.
Let should be const instead, also could this line be moved to be closer to where cmd is used? Maybe line 26
| // Determine which build runner to use | ||
| let cmd = process.env.MONOREPO_COMMAND || DEFAULT_COMMAND; | ||
|
|
||
| // Read environment variable (only relevant for monorepos with multiple targets) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Yep - I've also renamed the env var to "project" for more clarity
| let cmd = process.env.MONOREPO_COMMAND || DEFAULT_COMMAND; | ||
|
|
||
| // Read environment variable (only relevant for monorepos with multiple targets) | ||
| let target = process.env.MONOREPO_PROJECT || ""; |
| ): 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], { |
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
Ah yeah good catch - I thought npm build was still a valid alias for npm run build but not anymore. Will adjust accordingly
| @@ -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; | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Thanks for the fix Brian! I do think this change adds some additional tech debt to na already complex part of the flow, but given the time constraints I don't think it's feasible to address it now.
My one ask would be to add documentation where needed to detail any tradeoffs in design and potential cleanups so that this can be easily refactored in the future. Feel free to submit whenever Wei stamps LGTM
Builds that include configuration files at the root of the monorepo weren't being bundled into the generated artifacts because the adapter was running the monorepo build command from the project subdirectory context. We fix this by running build commands from the root and specifying the target project.
This PR also includes a change to read Nx project.json files to get and validate the Angular builder specified by the user. The Nx buildpack was previously responsible for reading this value from the project.json configuration file; however, to support use cases in which
FIREBASE_APP_DIRECTORYis not specified, we update the adapter to rely on Nx's workspace analysis mechanism to read the builder that was specified (which can only happen post-installation of the Nx dependency).