Support monorepo builds in Next.js adapter #160

blidd-google · 2024-04-10T19:54:35Z

Currently, the Next.js adapter is unable to specify a custom build command to run the Angular application builder. It also makes certain assumptions that no longer hold true in monorepo contexts. We adjust those conditions to enable builds of Next.js projects using monorepo tooling.

Yuangwang · 2024-04-10T22:08:54Z

packages/@apphosting/adapter-nextjs/src/utils.ts

  // dynamically load NextJS so this can be used in an NPX context
+  const require = createRequire(import.meta.url);


Why do we need this change? We might have discussed this but I forgot the reason

Yeah we discussed this a few weeks ago and my memory was also a bit fuzzy, but we need this in order to locate the package in the root node_modules folder (which is relevant for monorepo contexts).

Is there any additional documentation we can link to in this line comment?

Sorry not to be too pedantic but this comment explains what this does as opposed to why we need it.

Can we have a comment here explaining the motivation behind this statement? eg "Intentionally use ESM module resolution here to support blah blah blah"

See go/tott/563

A good litmus test is: Would someone who's not you, a year from now, understand why we need this statement based on the line comment? Would they be able to gather sufficient context either from the code, the comment, or linked docs to own it going forward?

tonyjhuang · 2024-04-11T13:19:03Z

packages/@apphosting/adapter-nextjs/src/bin/build.spec.ts

@@ -17,6 +18,13 @@ describe("build commands", () => {
      outputStaticDirectory: path.join(tmpDir, ".apphosting/.next/static"),
      serverFilePath: path.join(tmpDir, ".apphosting/server.js"),
    };
+    outputBundleMonorepoOptions = {


nit I would inline this, see go/tott/643

packages/@apphosting/adapter-nextjs/src/utils.ts

packages/@apphosting/adapter-nextjs/src/bin/build.ts

tonyjhuang · 2024-04-11T13:29:32Z

packages/@apphosting/adapter-nextjs/src/bin/build.ts

+const root = process.cwd();
+
+let projectRoot = root;
+if (process.env.MONOREPO_PROJECT && process.env.FIREBASE_APP_DIRECTORY) {


OOC why do we need the MONOREPO_PROJECT check? Shouldn't we always respect FIREBASE_APP_DIRECTORY?

Yes we should always be respecting FIREBASE_APP_DIRECTORY.

tonyjhuang · 2024-04-11T13:43:09Z

Overall, this seems like a reasonable approach and I see now that this follows the pattern used by the Angular adapter as well.

My high level feedback is: Any env vars that we introduce as inputs into the build adapter should be treated as a public API for other framework (adapter) authors down the road. We should make sure that this API is 1) thoroughly documented, and 2) follows our usual standards of API design including go/easy-to-use-and-hard-to-misuse (also see go/tott/523).

In particular, I would recommend re-examining our current set of env vars. As of right now, we have:

process.env.MONOREPO_PROJECT
process.env.FIREBASE_APP_DIRECTORY
process.env.MONOREPO_BUILDER
process.env.MONOREPO_COMMAND

It seems like some (MONOREPO_BUILDER) are not always used, while others (MONOREPO_PROJECT) can probably be removed all together. It also seems like a bug that FIREBASE_APP_DIRECTORY is only ever respected in MONOREPO contexts. IMO we should either rename it to MONOREPO_APP_DIRECTORY or always respect it.

tonyjhuang · 2024-04-11T13:46:09Z

I wonder if our API could be simplified down to:

process.env.MONOREPO_PROVIDER = "nx"?

This gives (community) adapter authors the most flexibility as opposed to having to handle our buildpack-specific logic such as MONOREPO_COMMAND, and we can have a shared util for example:

getBuildCommand(monorepoProvider) => providerToBuildCommand[monorepoProvider];

blidd-google · 2024-04-12T22:57:06Z

It seems like some (MONOREPO_BUILDER) are not always used

MONOREPO_BUILDER is not used in the Next.js buildpack, but is necessary for the Angular buildpack. The Nx buildpack is responsible for reading project config files (i.e. project.json), which is how we determine which application builder Angular is using in an Nx monorepo (in non-Nx environments we would read the angular.json, but there is no angular.json for Nx + Angular projects). We need some way to pass the information of which app builder is being used. The alternative would be to have the adapter read Nx project config files.

I do agree that MONOREPO_BUILDER is optional in many circumstances, however. This should be reflected in documentation.

while others (MONOREPO_PROJECT) can probably be removed all together

In the case of Nx, this is true - the MONOREPO_PROJECT is not strictly necessary because we can simply run nx run build from the context of the app directory and Nx will only build that app. Nx also supports nx run [project]:build to build a specific project. However, turbo run build by default builds every project regardless of the cwd context, and requires a --filter flag to run a build task for a single project (docs). So technically in the adapters, we should be using MONOREPO_PROJECT, but I haven't made the change yet.

It also seems like a bug that FIREBASE_APP_DIRECTORY is only ever respected in MONOREPO contexts. IMO we should either rename it to MONOREPO_APP_DIRECTORY or always respect it.

We should probably always respect it to support setups where the repo root != the app root.

tonyjhuang

Overall LGTM pending a few small nits.

My main ask following this CL is to see if we can encapsulate all of the env var wrangling in a single nx library that can be shareable across both the angular and the next adapters

tonyjhuang · 2024-04-15T19:22:00Z

packages/@apphosting/adapter-nextjs/src/utils.ts

  // dynamically load NextJS so this can be used in an NPX context
+  const require = createRequire(import.meta.url);


Is there any additional documentation we can link to in this line comment?

packages/@apphosting/adapter-nextjs/src/bin/build.ts

tonyjhuang · 2024-04-15T19:30:10Z

packages/@apphosting/adapter-nextjs/src/utils.ts

 // fs-extra is CJS, readJson can't be imported using shorthand
 export const { move, exists, writeFile, readJson } = fsExtra;

-export async function loadConfig(cwd: string) {
+// The default fallback command prefix to run a build.
+export const DEFAULT_COMMAND = "npm";


@Yuangwang might be able to provide more context, but my understanding is that we always run npm run build in non-monorepo scenarios to run the custom build command that a user has defined as their build script. Monorepo tools can be configured to run custom build scripts if desired (https://nx.dev/nx-api/nx/executors/run-script).

blidd-google · 2024-04-15T20:49:57Z

Overall LGTM pending a few small nits.

My main ask following this CL is to see if we can encapsulate all of the env var wrangling in a single nx library that can be shareable across both the angular and the next adapters

Sounds good - do you mean a single monorepo library?

blidd-google added 2 commits March 28, 2024 14:42

support nx w nextjs

aa582ec

mv static files to correct location

48a7943

blidd-google self-assigned this Apr 10, 2024

blidd-google requested review from Yuangwang, sjjj986 and jamesdaniels as code owners April 10, 2024 19:54

Yuangwang reviewed Apr 10, 2024

View reviewed changes

tonyjhuang requested changes Apr 11, 2024

View reviewed changes

fix nested non-monorepo builds & add docs

d5ad325

tonyjhuang reviewed Apr 15, 2024

View reviewed changes

add comment on createRequire

f963371

blidd-google added 2 commits April 16, 2024 14:42

update comment

05af7ba

edit comment

d88460b

tonyjhuang approved these changes Apr 16, 2024

View reviewed changes

blidd-google and others added 2 commits April 17, 2024 10:44

Merge branch 'main' into blidd-nextjs-adapter-monorepo

4091636

fix build error

01dd82b

blidd-google merged commit e258429 into main Apr 17, 2024
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support monorepo builds in Next.js adapter #160

Support monorepo builds in Next.js adapter #160

blidd-google commented Apr 10, 2024

Yuangwang Apr 10, 2024

blidd-google Apr 12, 2024

tonyjhuang Apr 15, 2024

tonyjhuang Apr 16, 2024

tonyjhuang Apr 16, 2024

tonyjhuang Apr 11, 2024

tonyjhuang Apr 11, 2024

blidd-google Apr 12, 2024

tonyjhuang commented Apr 11, 2024

tonyjhuang commented Apr 11, 2024

blidd-google commented Apr 12, 2024

tonyjhuang left a comment

tonyjhuang Apr 15, 2024

tonyjhuang Apr 15, 2024

blidd-google Apr 15, 2024 •

edited

Loading

blidd-google commented Apr 15, 2024

		// dynamically load NextJS so this can be used in an NPX context
		const require = createRequire(import.meta.url);

Support monorepo builds in Next.js adapter #160

Support monorepo builds in Next.js adapter #160

Conversation

blidd-google commented Apr 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonyjhuang commented Apr 11, 2024

tonyjhuang commented Apr 11, 2024

blidd-google commented Apr 12, 2024

tonyjhuang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blidd-google Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

blidd-google commented Apr 15, 2024

blidd-google Apr 15, 2024 •

edited

Loading