Refactor adapter build configuration logic into @apphosting/common module #204
Conversation
blidd-google
requested review from
Yuangwang,
sjjj986,
jamesdaniels,
tonyjhuang and
leoortizz
as code owners
| @@ -0,0 +1,25 @@ | |||
| // Options to configure the build of a framework application | |||
| export interface BuildOptions { | |||
| // command to run build script (e.g. "npm", "nx", etc.) | |||
There was a problem hiding this comment.
This may be too early to discuss but does this support go frameworks?
There was a problem hiding this comment.
At first glance I don't why not - the BuildOptions interface is essentially just allowing something (whether that's the monorepo buildpack or a go buildpack) to configure how the build is run.
| // passed down to the adapter from the buildpacks environment. | ||
| export function getBuildOptions(): BuildOptions { | ||
| return { | ||
| buildCommand: process.env.MONOREPO_COMMAND || DEFAULT_COMMAND, |
There was a problem hiding this comment.
Can we find a way to document the suppported environment variables?
There was a problem hiding this comment.
Publicly? I have a doc here: go/apphosting-monorepo-x-framework. It's a bit outdated, but it should be the canonical reference for the Monorepo Buildpack X Framework Adapter API.
| if (project) { | ||
| checkMonorepoBuildConditions(cmd, project); | ||
| if (opts.monorepoProject) { | ||
| checkMonorepoBuildConditions(opts.buildCommand, opts.monorepoProject); |
There was a problem hiding this comment.
Is it possible to refactor checkMonorepoBuildConditions out into the library as well? and keep it generalizable?
It would be great if we could reuse the monorepo support library for community adapters too, and for those I would probably want this to be as easy as possible to use.
lmk if this is just a problem in Angular and most other frameworks should be simpler. That would be ok then since we will have full control over the Angular adapter still
|
|
||
| // Get a set of default options, derived from the environment variable API | ||
| // passed down to the adapter from the buildpacks environment. | ||
| export function getBuildOptions(): BuildOptions { |
There was a problem hiding this comment.
Do you think getBuildOptions is generalizable to all other monorepos we may want to support too? If we want to support Turbo in the future for example, how would the turbo buildpack + this function interact?
|
|
||
| // Determine which command to run the build | ||
| const cmd = process.env.MONOREPO_COMMAND || DEFAULT_COMMAND; | ||
| const opts = getBuildOptions(); |
There was a problem hiding this comment.
I notice that in the nextjs case monorepo project is not used, do you know if that would be the more common pattern for other frameworks too?
|
|
||
| // Run build command from the subdirectory if specified. | ||
| // N.B. We run the build command from the root for monorepo builds, so that the build process can | ||
| // locate necessary files outside the project directory. | ||
| build(process.env.MONOREPO_CMD ? root : projectRoot, cmd, ...cmdArgs); | ||
| build(root, opts); |
There was a problem hiding this comment.
Would it be possible to simply this contract more on the adapter side? (This would probably complicate the getBuildOptions function).
I'm envisioning something like a new function buildMonorepoIfNeeded, that will handle 100% of monorepo related things, including the nx/turbo/... build. That way the adapter authors would need to know less about monorepos. This would help greatly in the case of community adapters where I'd be worried people might find monorepo support confusing.
I'm imagining something like this:
buildMonorepoIfNeeded(...){
// pull in the env vars the same way getBuildOptions does
// run the monorepo build if needed
return didMonorepoBuildRun, projectDirectory
}
then later to use it:
didMonorepoBuildRun, projectDirectory = buildMonorepoIfNeeded(...)
if !didMonorepoBuildRun{
build(root, opts)
}
then the rest is the same.
I like this way better since its more contained within a few lines and I think it might be easier for our docs to provide an example of how to use this
Do you think this would be possible and generalizable to other monorepo frameworks too? Also open to other ideas too if you think this can be simplified another way.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| export const DEFAULT_COMMAND = "npm"; | ||
|
|
||
| export async function runBuild(opts: BuildOptions = getBuildOptions()): Promise<BuildResult> { | ||
| const cmd = [opts.buildCommand, "run", "build", ...opts.buildArgs].join(" "); |
Check warning
Code scanning / CodeQL
Unsafe shell command constructed from library input Medium
| process.env.NEXT_PRIVATE_STANDALONE = "true"; | ||
| // Opt-out sending telemetry to Vercel | ||
| process.env.NEXT_TELEMETRY_DISABLED = "1"; | ||
| await runBuild(); |
There was a problem hiding this comment.
Do we want to support adapter authors who want to run a customized build command? Do we know that env vars are always sufficient to configure the build appropriately?
There was a problem hiding this comment.
Could you put together a small discovery doc of how other frameworks build commands work and whether this model would support them?
That would really help me understand the tradeoffs of this design
Burning down on some tech debt here from the pre-I/O push. Refactors the logic configuring build options into an npm module that can be published separately and shared between the adapters.