Conversation
|
Someone is attempting to deploy a commit to the Nitro Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdds EdgeOne deployment support: documentation, preset registration and types, an EdgeOne Nitro preset with build hooks, a runtime adapter converting EdgeOne Node.js requests to Nitro, and a utility that generates route metadata (meta.json) for API, prerendered, and SWR/cached routes. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
q153877011
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
src/presets/edgeone/runtime/edgeone.ts (1)
15-15: Unusedcontextparameter.The
contextparameter is declared but never used. If EdgeOne's bootstrap API requires this signature, consider prefixing with underscore (_context) to indicate it's intentionally unused, or document why it's present.🔎 Consider this diff:
-export default async function handle(req: EdgeOneRequest, context: unknown) { +export default async function handle(req: EdgeOneRequest, _context: unknown) {src/presets/edgeone/utils.ts (1)
68-77: Optimize SWR route matching performance.The nested iteration creates O(n*m) complexity. For better performance with many routes, consider using a Map to index
frameworkRoutesby path.🔎 Consider this optimization:
+ // Create a map for faster lookups + const routeMap = new Map( + meta.frameworkRoutes.map((route, index) => [route.path, index]) + ); + swrRouteRules.forEach((route) => { const maxAge = route.cache.maxAge; - meta.frameworkRoutes.forEach((frameworkRoute) => { - if (frameworkRoute.path === route.path) { - Reflect.set(frameworkRoute, "isStatic", false); - Reflect.set(frameworkRoute, "isr", maxAge); - } - }); + const index = routeMap.get(route.path); + if (index !== undefined) { + const frameworkRoute = meta.frameworkRoutes[index]; + Reflect.set(frameworkRoute, "isStatic", false); + Reflect.set(frameworkRoute, "isr", maxAge); + } });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
docs/2.deploy/20.providers/edgeone.md(1 hunks)src/presets/_all.gen.ts(2 hunks)src/presets/_types.gen.ts(1 hunks)src/presets/edgeone/preset.ts(1 hunks)src/presets/edgeone/runtime/_utils.ts(1 hunks)src/presets/edgeone/runtime/edgeone.ts(1 hunks)src/presets/edgeone/utils.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-03T19:09:13.905Z
Learnt from: medz
Repo: nitrojs/nitro PR: 3834
File: src/presets/cloudflare/server-entry.ts:63-63
Timestamp: 2025-12-03T19:09:13.905Z
Learning: In the Nitro Cloudflare preset (src/presets/cloudflare/server-entry.ts), native errors from oxc-parser and Node.js readFile operations should be allowed to bubble up naturally without wrapping, as their native error messages are more user-friendly and provide better diagnostic information.
Applied to files:
src/presets/edgeone/preset.tssrc/presets/_all.gen.ts
🧬 Code graph analysis (5)
src/presets/edgeone/utils.ts (1)
src/types/nitro.ts (1)
Nitro(16-37)
src/presets/edgeone/runtime/edgeone.ts (1)
src/presets/edgeone/runtime/_utils.ts (1)
normalizeHeaders(1-9)
src/presets/edgeone/runtime/_utils.ts (1)
src/runtime/internal/route-rules.ts (1)
headers(14-19)
src/presets/edgeone/preset.ts (3)
src/presets/_utils/preset.ts (1)
defineNitroPreset(6-18)src/types/nitro.ts (1)
Nitro(16-37)src/presets/edgeone/utils.ts (1)
writeEdgeOneRoutes(11-106)
src/presets/_types.gen.ts (1)
src/presets/index.ts (2)
PresetName(5-5)PresetNameInput(6-6)
🪛 Biome (2.1.2)
src/presets/edgeone/runtime/edgeone.ts
[error] 6-6: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🪛 LanguageTool
docs/2.deploy/20.providers/edgeone.md
[uncategorized] ~13-~13: The official name of this software platform is spelled with a capital “H”.
Context: ...oyment method. We support deployment on Github, GitLab, Gitee, and CNB. 3. Choose the ...
(GITHUB)
🔇 Additional comments (4)
src/presets/_types.gen.ts (1)
23-25: LGTM!The addition of "edgeone" to both
PresetNameandPresetNameInputtype unions is correct and aligns with the new EdgeOne preset implementation.src/presets/_all.gen.ts (1)
14-14: LGTM!The EdgeOne preset is correctly imported and exported following the established pattern for other presets.
Also applies to: 44-44
src/presets/edgeone/runtime/_utils.ts (1)
1-9: LGTM!The
normalizeHeadersfunction correctly converts header values to aHeadersInitcompatible format, properly handling undefined values and joining array values with comma-space separator as per HTTP standards.src/presets/edgeone/preset.ts (1)
5-29: LGTM!The EdgeOne preset is well-structured and follows Nitro preset conventions:
- Properly extends "node-server" as the base preset
- Configures appropriate output directories for EdgeOne deployment
- Generates route metadata during compilation via the
writeEdgeOneRouteshook
src/presets/edgeone/utils.ts
Outdated
| apiRoutes.forEach((route) => { | ||
| meta.frameworkRoutes.push({ | ||
| path: route.path, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Potential duplicate routes in frameworkRoutes.
API routes are added to frameworkRoutes (lines 33-37), and later prerender routes are also added (lines 87-92). If the same path exists in both collections, it will appear twice in frameworkRoutes. Consider checking for duplicates or using a Map/Set to track paths.
🔎 Consider this approach to prevent duplicates:
+ // Track added paths to prevent duplicates
+ const addedPaths = new Set<string>();
+
apiRoutes.forEach((route) => {
+ if (!addedPaths.has(route.path)) {
+ meta.frameworkRoutes.push({
+ path: route.path,
+ });
+ addedPaths.add(route.path);
+ }
- meta.frameworkRoutes.push({
- path: route.path,
- });
});
// ... later ...
allPrerenderRoutes.forEach((route) => {
+ if (!addedPaths.has(route)) {
+ meta.frameworkRoutes.push({
+ path: route,
+ isStatic: true,
+ });
+ addedPaths.add(route);
+ }
- meta.frameworkRoutes.push({
- path: route,
- isStatic: true,
- });
});Also applies to: 87-92
🤖 Prompt for AI Agents
In src/presets/edgeone/utils.ts around lines 33-37 (and similarly 87-92), API
and prerender routes are pushed into meta.frameworkRoutes without checking for
existing paths, causing duplicates; fix by tracking seen paths (e.g., a Set) and
only push a route if its path is not already present, or deduplicate
meta.frameworkRoutes after population by filtering unique path entries so each
path appears once.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/2.deploy/20.providers/edgeone.md (1)
22-22: Format the CLI command as code for better readability.The command
edgeone pages deployshould be formatted as inline code to follow standard documentation conventions and improve readability.🔎 Apply this diff to format the command:
-Once configured, use the edgeone pages deploy command to deploy the project. During deployment, the CLI will first automatically build the project, then upload and publish the build artifacts. +Once configured, use the `edgeone pages deploy` command to deploy the project. During deployment, the CLI will first automatically build the project, then upload and publish the build artifacts.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/2.deploy/20.providers/edgeone.md(1 hunks)src/presets/edgeone/runtime/edgeone.ts(1 hunks)src/presets/edgeone/utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/presets/edgeone/utils.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/presets/edgeone/runtime/edgeone.ts (1)
src/presets/edgeone/runtime/_utils.ts (1)
normalizeHeaders(1-9)
🪛 Biome (2.1.2)
src/presets/edgeone/runtime/edgeone.ts
[error] 6-6: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🔇 Additional comments (3)
src/presets/edgeone/runtime/edgeone.ts (3)
17-20: LGTM! URL construction is secure and robust.The URL construction correctly:
- Prioritizes EdgeOne-specific
eo-pages-hostheader- Falls back to standard
hostheader with a safe default- Defaults to
httpsprotocol for security (addressing previous review feedback)
22-27: Verify body handling for GET and HEAD requests.Request using GET or HEAD method cannot have a body; null is returned in these cases per the Fetch API specification. The code passes
req.bodyto the Request constructor for all HTTP methods, which will throw aTypeErrorif a non-empty body is provided with GET or HEAD requests.Please verify that:
- EdgeOne sets
req.bodytoundefined,null, or an empty string for GET and HEAD requests, or- Consider conditionally passing the body only for methods that support it
8-13: Thebodyproperty is strictly typed asstring, but the Web Request API's BodyInit type accepts multiple formats (string | Blob | ArrayBuffer | FormData | URLSearchParams | ReadableStream | null). Verify whether EdgeOne's runtime always provides request bodies as pre-buffered strings, or if the type should be broader to handle binary data and multipart form submissions that the platform may deliver.
|
Hey @q153877011 I noticed that in https://github.com/nitrojs/nitro/pull/3884/changes#diff-653eb6c213f3982301def6b933a232ceb95b4947cf50cdc70c5ed425223a192aR23 you created a new Web Request using basic information. The EdgeOne preset is based on the Node preset. Furthermore, EdgeOne's handler passes a Node IncomingMessage (https://nodejs.org/api/http.html#class-httpincomingmessage). Nitro relies on srvx, such as exported handlers. Could we directly use: export default toNodeHandler(nitroApp.fetch);Or, if a Request is required, we can use: const request = new NodeRequest({ req });PS: I haven't actually tested this on EdgeOne, but my intuition tells me this is the right approach. |
|
Additionally, I noticed that EdgeOne has its own Context, which I think should also be communicated to Nitro, for example, using |
|
Sorry, I might have gone too far with my comments 😂. This is because it's a concern of mine. When I saw this PR, I was thinking that besides deploying my application on Cloudflare/Vercel, EdgeOne might be a good option for providing services to users in China. So I carefully reviewed this PR and offered some of my thoughts. To summarize:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/presets/edgeone/runtime/edgeone.ts (1)
15-19: Consider exposing EdgeOne platform context to user code.Based on the PR feedback from medz, EdgeOne-specific context should be exposed to Nitro via
event.context.edgeoneso users can access platform-specific features. Currently, the EdgeOne context parameter is not being forwarded to Nitro's event system.If EdgeOne provides a context object with platform-specific data (environment variables, request metadata, etc.), consider passing it through the NodeRequest constructor or attaching it to the Nitro event after request construction. This would align with existing patterns like
event.context.cloudflarein other presets.Based on PR feedback from medz.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/2.deploy/20.providers/edgeone.mdplayground/nitro.config.tssrc/presets/_types.gen.tssrc/presets/edgeone/preset.tssrc/presets/edgeone/runtime/edgeone.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/presets/_types.gen.ts
- docs/2.deploy/20.providers/edgeone.md
- src/presets/edgeone/preset.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-03T19:09:13.905Z
Learnt from: medz
Repo: nitrojs/nitro PR: 3834
File: src/presets/cloudflare/server-entry.ts:63-63
Timestamp: 2025-12-03T19:09:13.905Z
Learning: In the Nitro Cloudflare preset (src/presets/cloudflare/server-entry.ts), native errors from oxc-parser and Node.js readFile operations should be allowed to bubble up naturally without wrapping, as their native error messages are more user-friendly and provide better diagnostic information.
Applied to files:
playground/nitro.config.ts
🪛 Biome (2.1.2)
src/presets/edgeone/runtime/edgeone.ts
[error] 6-6: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🔇 Additional comments (1)
playground/nitro.config.ts (1)
4-4: LGTM! Playground configuration updated for EdgeOne testing.The preset name
edgeone-pagesaligns with the new preset registration and type definitions introduced in this PR.
| import { useNitroApp } from "nitro/app"; | ||
| import type { IncomingMessage } from "node:http"; | ||
|
|
||
| const nitroApp = useNitroApp(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Move useNitroApp() call inside the handler function.
Calling useNitroApp() at module scope may cause timing issues if the Nitro app is not fully initialized when this module loads. Other Nitro presets typically call useNitroApp() inside the handler function to ensure the app is ready.
🔎 Proposed fix
-const nitroApp = useNitroApp();
-
interface EdgeOneRequest extends IncomingMessage {
url: string;
method: string;
headers: Record<string, string | string[] | undefined>;
}
// EdgeOne bootstrap expects: async (req, context) => Response
export default async function handle(req: EdgeOneRequest) {
+ const nitroApp = useNitroApp();
// Use srvx NodeRequest to convert Node.js request to Web Request
const request = new NodeRequest({ req });🧰 Tools
🪛 Biome (2.1.2)
[error] 6-6: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🤖 Prompt for AI Agents
In src/presets/edgeone/runtime/edgeone.ts around line 6, the call to
useNitroApp() is made at module scope; move this call into the handler function
so the Nitro app is resolved at request time. Update the handler to call const
nitroApp = useNitroApp() at the start of the handler (or lazily when first
needed) and replace the module-scoped reference with this local variable; ensure
no other module-level code depends on nitroApp and adjust imports/refs
accordingly.
| // EdgeOne bootstrap expects: async (req, context) => Response | ||
| export default async function handle(req: EdgeOneRequest) { |
There was a problem hiding this comment.
Add the missing context parameter to match EdgeOne's handler signature.
The comment indicates EdgeOne expects async (req, context) => Response, but the function signature only accepts req. If EdgeOne passes a context object, it should be captured in the signature to avoid runtime errors and enable potential use of platform-specific context.
🔎 Proposed fix
-// EdgeOne bootstrap expects: async (req, context) => Response
-export default async function handle(req: EdgeOneRequest) {
+// EdgeOne bootstrap expects: async (req, context) => Response
+export default async function handle(req: EdgeOneRequest, context?: any) {
const nitroApp = useNitroApp();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // EdgeOne bootstrap expects: async (req, context) => Response | |
| export default async function handle(req: EdgeOneRequest) { | |
| // EdgeOne bootstrap expects: async (req, context) => Response | |
| export default async function handle(req: EdgeOneRequest, context?: any) { | |
| const nitroApp = useNitroApp(); |
🤖 Prompt for AI Agents
In src/presets/edgeone/runtime/edgeone.ts around lines 14-15 the default export
handler only declares a single parameter (req) but EdgeOne expects the signature
async (req, context) => Response; modify the function declaration to accept the
second parameter (context) — e.g., async function handle(req, context) — and if
available, add an appropriate type for context (EdgeOneContext or any) to the
signature so the runtime-provided context is captured without altering handler
behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/presets/edgeone/runtime/edgeone.ts (2)
6-6: MoveuseNitroApp()call inside the handler function.Calling
useNitroApp()at module scope can cause timing issues if the Nitro app is not fully initialized when this module loads. Other Nitro presets calluseNitroApp()inside the handler to ensure the app is ready.🔎 Proposed fix
-const nitroApp = useNitroApp(); - interface EdgeOneRequest extends IncomingMessage { url: string; method: string; headers: Record<string, string | string[] | undefined>; } // EdgeOne bootstrap expects: async (req, context) => Response export default async function handle(req: EdgeOneRequest) { + const nitroApp = useNitroApp(); // Use srvx NodeRequest to convert Node.js request to Web Request const request = new NodeRequest({ req });
14-15: Add the missingcontextparameter to match EdgeOne's handler signature.The comment indicates EdgeOne expects
async (req, context) => Response, but the function signature only acceptsreq. If EdgeOne passes a context object, it should be captured to avoid potential runtime issues and enable use of platform-specific context.🔎 Proposed fix
// EdgeOne bootstrap expects: async (req, context) => Response -export default async function handle(req: EdgeOneRequest) { +export default async function handle(req: EdgeOneRequest, context?: any) { const nitroApp = useNitroApp();
🧹 Nitpick comments (2)
src/presets/edgeone/runtime/edgeone.ts (2)
8-12: Consider simplifying the EdgeOneRequest interface.The interface redeclares properties (
url,method,headers) that already exist onIncomingMessage. If the intent is to make these properties required or adjust their types, consider documenting why, or simply useIncomingMessagedirectly if the type differences aren't needed.
16-19: Consider exposing EdgeOne platform context to user code.The current implementation uses
NodeRequestcorrectly (aligning with reviewer medz's suggestions), but doesn't expose EdgeOne-specific context to Nitro. Consider capturing the EdgeOnecontextparameter and making it available viaevent.context.edgeone, similar to how other presets expose platform context (e.g.,event.context.cloudflare). This would allow users to access EdgeOne-specific features in their handlers.Based on reviewer medz's feedback in PR objectives.
Example approach
After capturing the
contextparameter, you could pass it to the request creation:const request = new NodeRequest({ req, context: { edgeone: context } });Or use Nitro's event context mechanism to attach it, depending on how
NodeRequesthandles context propagation.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/2.deploy/20.providers/edgeone.mdsrc/presets/_types.gen.tssrc/presets/edgeone/preset.tssrc/presets/edgeone/runtime/edgeone.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/presets/_types.gen.ts
- docs/2.deploy/20.providers/edgeone.md
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: medz
Repo: nitrojs/nitro PR: 3834
File: src/presets/cloudflare/server-entry.ts:63-63
Timestamp: 2025-12-03T19:09:13.905Z
Learning: In the Nitro Cloudflare preset (src/presets/cloudflare/server-entry.ts), native errors from oxc-parser and Node.js readFile operations should be allowed to bubble up naturally without wrapping, as their native error messages are more user-friendly and provide better diagnostic information.
📚 Learning: 2025-12-03T19:09:13.905Z
Learnt from: medz
Repo: nitrojs/nitro PR: 3834
File: src/presets/cloudflare/server-entry.ts:63-63
Timestamp: 2025-12-03T19:09:13.905Z
Learning: In the Nitro Cloudflare preset (src/presets/cloudflare/server-entry.ts), native errors from oxc-parser and Node.js readFile operations should be allowed to bubble up naturally without wrapping, as their native error messages are more user-friendly and provide better diagnostic information.
Applied to files:
src/presets/edgeone/runtime/edgeone.tssrc/presets/edgeone/preset.ts
🪛 Biome (2.1.2)
src/presets/edgeone/runtime/edgeone.ts
[error] 6-6: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🔇 Additional comments (1)
src/presets/edgeone/preset.ts (1)
5-29: Preset configuration looks good.The preset configuration appropriately extends "node-server", enables static file serving, and sets up correct output directories. The name is explicitly set to "edgeone-pages" (line 27), addressing the naming consideration from past reviews.
Hello @medz @gxres042. Changes have been made based on your feedback(bd99387c6231ee66eaef2b8675a4a57a43b67e37):
|
pi0
changed the title
edgeone preset
|
|
||
| export async function writeEdgeOneRoutes(nitro: Nitro) { | ||
| // Ensure routes are synced | ||
| nitro.routing.sync(); |
There was a problem hiding this comment.
Is calling routing.sync() in preset really necessary? 🤔
My small backend project based on Nitro （* v3) can deploy to EdgeOne Pages properly without this.
There was a problem hiding this comment.
During our CD process, routing is allocated based on the request path. I call this to prevent route loss issues in more complex projects.
|
@pi0 May I ask when this content could be officially published? We have some upcoming ecosystem deployment work in conjunction with Nitro that we need to schedule accordingly. |
| const request = new NodeRequest({ req }); | ||
|
|
||
| return nitroApp.fetch(request); |
There was a problem hiding this comment.
This looks a very strange signature mixing node rew as incoming and web Response (result of fetch) as outgoing. Can you please confirm that:
- You pass a fully compliant Node.js
IncomingMessage. - Accept a fully compliant Web
Responseobject
Is there an alternative that both req and res are the same format (either Node or Web, but web req is prefered)?
| apiRoutes, | ||
| pageRoutes, |
There was a problem hiding this comment.
Can you please clarify, what is difference between handling api routes and pages routes in platform? (possibly linking relavant docs to code might be useful)
Additionally, (also important) we need to register a fallback route that handles any uncaught route pattern (middleware for example can also catch routes)
pi0
left a comment
pi0
left a comment
There was a problem hiding this comment.
@q153877011 thanks for PR ❤️
Here are next steps before merging support into v3:
- Finalizing discussion on handler signature (Web or Node)
- Possibly adding preset tests
- Finalizing discussion on routing (catch-all)
- Setting a continuous E2E deployment in nitro-deploys (i can do this later)
- Possibly auto-detecting edge-one pages CI using a known environment variable for zero config (to be added here (optional, only if you have already one)
Feel free to message me in discord (@pi0) for quick discusison.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/presets/edgeone/runtime/edgeone.ts (2)
6-6: Consider movinguseNitroApp()inside the handler.Calling
useNitroApp()at module scope may cause timing issues if the Nitro app isn't fully initialized when the module loads. While the Biome warning about React hooks is a false positive (this isn't React), the underlying concern about initialization order is valid for Nitro presets.🔎 Proposed fix
-const nitroApp = useNitroApp(); - interface EdgeOneRequest extends IncomingMessage { // ... } export default async function handle(req: EdgeOneRequest) { + const nitroApp = useNitroApp(); const request = new NodeRequest({ req }); return nitroApp.fetch(request); }
14-15: Add the missingcontextparameter to match EdgeOne's expected handler signature.The comment on line 14 indicates EdgeOne expects
async (req, context) => Response, but the function only acceptsreq. Capturing the context parameter would enable access to platform-specific features (as suggested in PR comments about exposingevent.context.edgeone).🔎 Proposed fix
-export default async function handle(req: EdgeOneRequest) { +export default async function handle(req: EdgeOneRequest, _context?: unknown) { const nitroApp = useNitroApp();src/presets/edgeone/utils.ts (1)
34-38: Potential duplicate routes inframeworkRoutes.API routes are added to
frameworkRoutesat lines 34-38, and prerender routes are appended at lines 86-91. If the same path exists in both collections (e.g., an API route that's also prerendered), it will appear twice in the outputmeta.json. Consider deduplicating or using a Map keyed by path.🔎 Proposed fix using a Set to track added paths
+ const addedPaths = new Set<string>(); + for (const route of apiRoutes) { + if (addedPaths.has(route.path)) continue; meta.frameworkRoutes.push({ path: route.path, }); + addedPaths.add(route.path); } // ... later in the function ... for (const route of allPrerenderRoutes) { + if (addedPaths.has(route)) continue; meta.frameworkRoutes.push({ path: route, isStatic: true, }); + addedPaths.add(route); }Also applies to: 86-91
🧹 Nitpick comments (2)
src/presets/edgeone/utils.ts (2)
113-116:prettyPathfunction is currently unused.The
prettyPathhelper is defined butwriteFileis always called withlog = false(the default), so the logging path that usesprettyPathis never executed. Consider either:
- Using
log = truefor at least one call to provide user feedback- Removing the function if logging is intentionally silent
72-73: Prefer direct property assignment overReflect.set.Using
Reflect.setfor simple property assignment on a plain object is unconventional and less readable. Direct assignment is clearer and has the same effect here.🔎 Proposed simplification
if (frameworkRoute.path === route.path) { - Reflect.set(frameworkRoute, "isStatic", false); - Reflect.set(frameworkRoute, "isr", maxAge); + frameworkRoute.isStatic = false; + (frameworkRoute as FrameworkRoute & { isr?: number }).isr = maxAge; }Alternatively, add
isrto theFrameworkRouteinterface since it's being used:interface FrameworkRoute { path: string; isStatic?: boolean; + isr?: number; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/presets/edgeone/preset.tssrc/presets/edgeone/runtime/edgeone.tssrc/presets/edgeone/utils.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: medz
Repo: nitrojs/nitro PR: 3834
File: src/presets/cloudflare/server-entry.ts:63-63
Timestamp: 2025-12-03T19:09:13.905Z
Learning: In the Nitro Cloudflare preset (src/presets/cloudflare/server-entry.ts), native errors from oxc-parser and Node.js readFile operations should be allowed to bubble up naturally without wrapping, as their native error messages are more user-friendly and provide better diagnostic information.
📚 Learning: 2025-12-03T19:09:13.905Z
Learnt from: medz
Repo: nitrojs/nitro PR: 3834
File: src/presets/cloudflare/server-entry.ts:63-63
Timestamp: 2025-12-03T19:09:13.905Z
Learning: In the Nitro Cloudflare preset (src/presets/cloudflare/server-entry.ts), native errors from oxc-parser and Node.js readFile operations should be allowed to bubble up naturally without wrapping, as their native error messages are more user-friendly and provide better diagnostic information.
Applied to files:
src/presets/edgeone/runtime/edgeone.tssrc/presets/edgeone/preset.ts
🧬 Code graph analysis (2)
src/presets/edgeone/utils.ts (1)
src/types/nitro.ts (1)
Nitro(16-37)
src/presets/edgeone/preset.ts (3)
src/presets/_utils/preset.ts (1)
defineNitroPreset(6-18)src/types/nitro.ts (1)
Nitro(16-37)src/presets/edgeone/utils.ts (1)
writeEdgeOneRoutes(12-111)
🪛 Biome (2.1.2)
src/presets/edgeone/runtime/edgeone.ts
[error] 6-6: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🔇 Additional comments (3)
src/presets/edgeone/preset.ts (1)
5-29: LGTM! Well-structured preset definition.The preset correctly extends
node-server, configures appropriate output directories for EdgeOne's expected structure, and hooks into thecompiledlifecycle to generate route metadata. The naming asedgeone-pagesaligns with the PR discussion.src/presets/edgeone/runtime/edgeone.ts (1)
1-20: Overall runtime implementation looks correct.The use of
srvx/node'sNodeRequestto convert the Node.jsIncomingMessageto a Web Request is the recommended approach per PR discussion (medz's suggestion to rely on srvx instead of custom conversion). The flow is clean: polyfills → convert request → delegate tonitroApp.fetch().src/presets/edgeone/utils.ts (1)
12-110: Overall implementation is solid.The function correctly extracts route metadata from multiple Nitro sources (API routes, prerendered pages, user-defined prerender routes, route rules with prerender/SWR settings) and writes a structured
meta.jsonfor EdgeOne's deployment pipeline. The return value provides useful debugging information.
| for (const route of swrRouteRules) { | ||
| const maxAge = route.cache.maxAge; | ||
| for (const frameworkRoute of meta.frameworkRoutes) { | ||
| if (frameworkRoute.path === route.path) { | ||
| Reflect.set(frameworkRoute, "isStatic", false); | ||
| Reflect.set(frameworkRoute, "isr", maxAge); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
SWR routes may not match if added after API routes.
The SWR/ISR annotation loop (lines 68-76) iterates over meta.frameworkRoutes to find matching paths and update them with isStatic: false and isr: maxAge. However, this only works for routes already added to frameworkRoutes (i.e., API routes). If a SWR route path isn't an API route but comes from prerender routes (added later at line 86-91), it won't be annotated with ISR settings.
Consider reordering: collect all routes first, then apply SWR annotations, or apply SWR settings when adding prerender routes.
🤖 Prompt for AI Agents
In src/presets/edgeone/utils.ts around lines 68 to 76, the loop that annotates
SWR/ISR on meta.frameworkRoutes runs before prerender routes are appended (lines
~86-91), so SWR paths added later won't get isStatic/isr set; fix by either
moving the SWR annotation block to run after all routes are collected (i.e.,
after prerender routes are added) or, when adding prerender routes, apply the
SWR settings there by looking up the matching swrRouteRules and setting
isStatic/isr on the newly pushed route so all routes receive ISR annotations
regardless of insertion order.
Happy New Year @pi0. Below are my responses to the two questions you raised:
API routes: All API routes are deployed to cloud functions. When an incoming request matches an existing API route, we route it directly to the corresponding cloud function as the origin. Pages routes:SSR/ISR routes are routed back to a Node.js cloud function to run the rendering logic. Unmatched routes (e.g. static assets, CDN images) are handled by other layers in our architecture.
I agree it looks a bit odd, but we can’t change it for now because this handler is part of our CI pipeline. Once the CI workflow stabilizes, we plan to move more complete functionality into the preset, and this part should look more reasonable then. |
|
Thanks for the explanation. Current architecture of Nitro output is a single server handler (even for multi-function platforms like vercel, we use one function with lazy-loaded chunks). This allows sharing lazy-loaded chunks. It would be best if we follow same for edgeone perhaps with a flag (so all routes are either handled in cloud functions or Node.js handler) Re format, if it is really what you need, (node req, web res) that's fine we might only need to e2e test it and add some refs for later refactor. |
Yes, that’s correct—no problem. Right now, everything is handled through a single Node.js handler entry point. My previous reply may have misled you. |
|
@pi0 Hello, do you have any further questions regarding the pull request submitted for EdgeOne's presets? And is the next step testing? What is the expected testing period?😁 |
I would like to ask, does this support edge functions? Or can I switch between edge and nodejs functions? |
|
The goal is to be able to switch between two. (on delay) I had been struggling to find something to dig more, but unfortunately, there weren't enough docs (even searched chinese versi, which had more details) |
@cheezone .Yes, we support edge functions. If developers want to switch manually, they can create a For Nuxt Here is our documentation: https://pages.edgeone.ai/document/pages-functions-overview |
https://pages.edgeone.ai/document/pages-functions-overview This is our documentation, and we’re not sure if it fully answers your question. Users can manually switch the execution environment by creating function files, or they can directly use Nuxt’s The compilation of Node functions and Edge functions is currently handled by the EdgeOne CLI. After things become more stable, we plan to integrate this into Nitro (similar to how Vercel does it). |
Is it possible to specify a partial route to the edge and a part to the node? I see in your documentation that Node Functions supports processing and responding to WebSocket upgrade requests to create persistent two-way communication connections. Developers can view basic log information for Node Functions calls. So it doesn't support WebSockets and logs at the edge like Cloudflare. However, if you use Node Functions, you cannot use products such as EdgeOne's KV. Is this something you guys will do in the future, or is Nuxt's '/api/' routes only planned to deploy to Node.js environments? |
Your questions are very professional; here are the answers:
Nuxt’s The issues you mentioned have also been raised by our users, and we will schedule them for improvement in future updates. I will also organize your professional feedback and report it to my superiors, hoping that these issues can be resolved as soon as possible.😊 |
|
Thanks for the link to the docs, @q153877011. However, docs does not cover the interfaces that are currently implemented, such as:
Where are these interfaces documented? It is also not clear where meta.json should be written. Is this documented anywhere? About the output target (Node or Edge): Nitro already has a built-in edge compatibility layer (unenv) that handles many limitations and is used for Cloudflare. Once I understand the interfaces and their documentation, I can help much more. |
5 participants
🔗 Linked issue
#2973
❓ Type of change
📚 Description
Hi, I’m a developer of EdgeOne Pages. In this PR, I’m adding a preset to support deploying Nitro projects on EdgeOne Pages.
I’ve completed the implementation and deployment testing. This update includes some configuration for the Nitro build package, while additional platform-side network request settings have been moved into the platform CI workflow.
Please review. Thanks.