Playing around with native adapter #456
Conversation
Yuangwang
requested review from
abhis3,
annajowang,
jamesdaniels,
leoortizz,
sjjj986 and
taeold
as code owners
Summary of ChangesHello @Yuangwang, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the Next.js adapter for the app hosting platform, moving towards a more 'native' integration with Next.js. The changes leverage Next.js's built-in standalone output and internal server capabilities, streamlining the deployment and serving of Next.js applications. The new architecture aims to improve compatibility and performance by relying more directly on Next.js's own mechanisms for building and serving. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Yuangwang
removed request for
abhis3,
annajowang,
jamesdaniels,
leoortizz,
sjjj986 and
taeold
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the Next.js adapter to use the new experimental Next.js Adapter API, which is a great improvement. The new approach is cleaner and leverages framework-native features for build-time configuration and post-build steps.
My review includes several suggestions to improve type safety, maintainability, and fix a potential bug in a build script. Specifically, I've pointed out:
- A misconfigured build command in
package.json. - An unused dependency.
- Opportunities to improve type safety in
serve.tsandindex.ts. - A potentially problematic piece of code in
serve.tsthat is marked for local testing but runs in production. - A case of silent error handling that could be improved.
Additionally, with the move to the new adapter API, it appears that src/utils.ts and src/overrides.ts are no longer used and could potentially be removed in a follow-up change to clean up the codebase.
Overall, this is a solid step forward for the adapter. Addressing these points will make the implementation more robust and maintainable.
| "scripts": { | ||
| "build": "rm -rf dist && tsc && chmod +x ./dist/bin/*", | ||
| "build": "rm -rf dist && tsc && npm run bundle", | ||
| "bundle": "esbuild src/bin/build.ts --bundle --platform=node --format=esm --outfile=dist/bin/build.js --external:esbuild --external:fs-extra && esbuild src/bin/serve.ts --bundle --platform=node --format=esm --outfile=dist/bin/serve.js --external:next --external:react --external:react-dom && esbuild src/index.ts --bundle --platform=node --format=cjs --outfile=dist/index.cjs", |
There was a problem hiding this comment.
The bundle script for serve.ts specifies --format=esm, but src/bin/serve.ts uses CommonJS features like require() and require.resolve(). This will cause a runtime error when the apphosting-adapter-nextjs-serve binary is executed.
To ensure compatibility, the output format should be CommonJS (cjs).
| "bundle": "esbuild src/bin/build.ts --bundle --platform=node --format=esm --outfile=dist/bin/build.js --external:esbuild --external:fs-extra && esbuild src/bin/serve.ts --bundle --platform=node --format=esm --outfile=dist/bin/serve.js --external:next --external:react --external:react-dom && esbuild src/index.ts --bundle --platform=node --format=cjs --outfile=dist/index.cjs", | |
| "bundle": "esbuild src/bin/build.ts --bundle --platform=node --format=esm --outfile=dist/bin/build.js --external:esbuild --external:fs-extra && esbuild src/bin/serve.ts --bundle --platform=node --format=cjs --outfile=dist/bin/serve.js --external:next --external:react --external:react-dom && esbuild src/index.ts --bundle --platform=node --format=cjs --outfile=dist/index.cjs", |
| await server.prepare(); | ||
|
|
||
| // 5. Start HTTP Listener | ||
| createServer(async (req: any, res: any) => { |
There was a problem hiding this comment.
The request (req) and response (res) objects are typed as any. This bypasses TypeScript's type checking. For better type safety and code clarity, you should use the specific types from Node's http module.
First, import them:
import { createServer, IncomingMessage, ServerResponse } from "http";Then, use them in the function signature:
| createServer(async (req: any, res: any) => { | |
| createServer(async (req: IncomingMessage, res: ServerResponse) => { |
| // FAKE PROXY HEADER (For local testing only) | ||
| if (!req.headers['x-matched-path']) { | ||
| req.headers['x-matched-path'] = parsedUrl.pathname; | ||
| } |
There was a problem hiding this comment.
The comment states that faking the x-matched-path header is "For local testing only", but this code is part of the production server and will run in all environments (since NODE_ENV is hardcoded to production). This is misleading and could cause unexpected behavior in production if this header influences Next.js routing.
If this logic is not needed in production, it should be removed. If it is for local testing, it should be conditionally executed based on a different environment variable.
| "license": "Apache-2.0", | ||
| "dependencies": { | ||
| "@apphosting/common": "*", | ||
| "fastify": "^5.6.1", |
There was a problem hiding this comment.
The fastify dependency was added, but it doesn't appear to be used anywhere in the codebase. The new serve.ts uses Node's built-in http module. To keep dependencies clean, this unused package should be removed.
| adapterMetadata, | ||
| ); | ||
| // Copy the 'public' folder and '.next/static' (Standalone doesn't include these by default!) | ||
| await fs.copy(join(root, "public"), join(outputDir, "public"), { dereference: true }).catch(() => {}); |
There was a problem hiding this comment.
Silently catching all errors with .catch(() => {}) when copying the public directory can hide important build failures, such as permission issues. While the public directory may not always exist, it's better to explicitly check for its existence before copying rather than ignoring all potential errors. Consider refactoring to check for the directory's existence first.
| const server = new NextServer({ | ||
| dir: currentDir, | ||
| hostname: '0.0.0.0', | ||
| port: parseInt(process.env.PORT || "8080"), |
There was a problem hiding this comment.
The logic for parsing the port number, parseInt(process.env.PORT || "8080"), is used multiple times (here and on line 56). To improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle, this value should be calculated once and stored in a constant at the beginning of the start function.
For example:
const port = parseInt(process.env.PORT || "8080");| }, | ||
|
|
||
| // Hook 2: Build Complete (The specific feature you wanted) | ||
| async onBuildComplete(data) { |
There was a problem hiding this comment.
The data parameter in the onBuildComplete hook is untyped. Providing a type would improve code clarity and leverage TypeScript's static analysis. You can import NextBuildCompleteData from next and use it here.
| async onBuildComplete(data) { | |
| async onBuildComplete(data: NextBuildCompleteData) { |
1 participant
No description provided.