Use Marko API of top layout for route entry templates#179
Conversation
🦋 Changeset detectedLatest commit: 0687d97 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughAdds a changeset for a patch release to packages/run. CI workflow tooling upgraded (actions/checkout and actions/setup-node versions; Node 24 added) and release job permissions adjusted. Wide dependency bumps across root, packages, and examples. Introduces new test fixtures (force-class-api, force-tags-api) including marko.json, transform.ts, tsconfig.json, +layout.marko, +page.marko, and generated .marko-run typings. Vite plugin and codegen now resolve a per-route Marko API with caching; renderRouteTemplate and renderRouter signatures changed. Test utilities updated (createDirectory signature extended; createTestWalker signature simplified). Many vite fixtures set server.hmr = false. CLI getViteConfig gained a fallback parameter; server close now awaits process exit. Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/run/src/vite/__tests__/main.test.ts (1)
39-57: If a fixture has multipleroutes*.txtfiles, metadata lookups will be wrong (only lasttestFskept).
sourcescan contain multiple walkers (one per routes file), buttestFsis overwritten each time; laterroute.handler.filePathmay point into a different source tree than the retainedmetamap. Suggest aggregating meta across allroutes*.txtinputs (or storing meta per source and selecting by prefix).- let testFs: ReturnType<typeof createDirectory> | undefined; + let fileTreeForWalker: ReturnType<typeof createDirectory>["fileTree"] | undefined; + const meta = new Map<string, Record<string, any>>(); @@ - testFs = createDirectory(src, routesDir); + const fsPart = createDirectory(src, routesDir); + fileTreeForWalker = fsPart.fileTree; + for (const [k, v] of fsPart.meta) meta.set(k, v); sources.push({ - walker: createTestWalker(testFs.fileTree), + walker: createTestWalker(fsPart.fileTree), basePath: match[1], }); @@ - const meta = testFs?.meta.get(route.handler.filePath) as + const handlerMeta = meta.get(route.handler.filePath) as | { verbs: HttpVerb[] } | undefined; - const verbs = meta?.verbs.filter((v) => httpVerbs.includes(v)); + const verbs = handlerMeta?.verbs?.filter?.((v) => httpVerbs.includes(v)); route.handler.verbs = verbs?.length ? verbs : (httpVerbs as any); @@ - const layoutMeta = testFs?.meta.get(route.layouts[0].filePath) as + const layoutMeta = meta.get(route.layouts[0].filePath) as | { api: string } | undefined;Also applies to: 100-106, 111-120
♻️ Duplicate comments (2)
packages/run/src/__tests__/fixtures/force-class-api/transform.ts (1)
1-40: Same compatibility considerations as the tags transform (lookbehind +@marko/compilerAST APIs).packages/run/src/__tests__/fixtures/force-tags-api/.marko-run/routes.d.ts (1)
6-39: Please verify these generated typings align with@marko/run@0.9.3entrypoints and resolution rules.
🧹 Nitpick comments (8)
packages/run/src/__tests__/fixtures/force-tags-api/tsconfig.json (1)
1-4: Consider widening.marko-runinclude glob if nested output is possible.
If.marko-runcan contain subfolders, prefer.marko-run/**/*to avoid missing types during fixture compilation.packages/run/src/__tests__/fixtures/force-class-api/tsconfig.json (1)
1-4: Consider.marko-run/**/*instead of.marko-run/*(if nested output can occur).packages/run/src/__tests__/fixtures/force-class-api/src/routes/+layout.marko (1)
1-14: Fixture is clear; consider guardinginput.renderBodyif this pattern spreads beyond tests.In a layout,
input.renderBodyshould be present, but if this snippet gets copied elsewhere, a missingrenderBodycould fail at runtime—optional defensive check (or a comment) may prevent future misuse.packages/run/src/__tests__/fixtures/force-tags-api/transform.ts (1)
3-40: Test-fixture duplication withforce-class-api/transform.ts.If these are intended to stay identical long-term, consider sharing a single transform implementation (or generating both) to avoid drift.
packages/run/src/vite/__tests__/utils/fakeFS.ts (2)
10-12: Metadata JSON parsing needs error context (line/path) to debug fixture failures.
A malformed{ ... }will throw fromJSON.parsewithout indicating which fixture line/path caused it. Wrap with a try/catch and rethrow withlineNoand the computed key/name.const match = matchMeta.exec(name); if (match) { name = name.slice(0, match.index); - meta.set( - `${dirStack.join("/")}/${name}`, - JSON.parse(match[1].slice(1)), - ); + const key = path.join(...dirStack, name); + try { + meta.set(key, JSON.parse(match[1].slice(1))); + } catch (e) { + throw new Error( + `Line ${lineNo}: invalid metadata JSON for '${key}': ${String(e)}`, + ); + } } current[1].push(name); }Also applies to: 52-58
23-25: Only the first tab is expanded; consider replacing all tabs.
If a fixture line includes multiple tabs, indentation can parse incorrectly.- const line = match[1] - .replace(/\t/, " ") + const line = match[1] + .replace(/\t/g, " ") .match(/^(\s*)([/]?)([^\s/].*)\s*/);packages/run/src/vite/__tests__/main.test.ts (1)
101-106: Guard meta shapes to avoid test crashes on bad fixtures.
The casts assume{ verbs: HttpVerb[] }and{ api: string }. If a fixture provides wrong types (or meta key mismatch),meta?.verbs.filter(...)can throw. UseArray.isArray(handlerMeta?.verbs)andtypeof layoutMeta?.api === "string".Also applies to: 113-120
packages/run/src/vite/routes/walk.ts (1)
70-71: Edge case: ifdir[0] === "", generated paths become absolute-ish (/file) rather than relative.
If you intend to supportbaseDir=""test trees, consider special-casing prefix creation (only addpath.sepwhencurrentPathis non-empty).- const prefix = currentPath + path.sep; + const prefix = currentPath ? currentPath + path.sep : ""; @@ - await walk(dir, dir[0], maxDepth); + await walk(dir, dir[0], maxDepth);Also applies to: 77-86, 108-109
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (38)
package-lock.jsonis excluded by!**/package-lock.jsonand included by**packages/run/src/__tests__/fixtures/force-class-api/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/force-class-api/__snapshots__/preview.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/force-tags-api/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/force-tags-api/__snapshots__/preview.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/basic-500/__snapshots__/basic-500.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/basic-500/__snapshots__/basic-500.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/basic/__snapshots__/basic.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/basic/__snapshots__/basic.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/build-routes/__snapshots__/build-routes.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/build-routes/__snapshots__/build-routes.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/build-routes/__snapshots__/build-routes.expected.routetypes.d.tsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/escape-path/__snapshots__/escape-path.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/escape-path/__snapshots__/escape-path.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/flat-routes/__snapshots__/flat-routes.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/flat-routes/__snapshots__/flat-routes.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/flat-routes/__snapshots__/flat-routes.expected.routetypes.d.tsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-class-api/__snapshots__/force-class-api.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-class-api/__snapshots__/force-class-api.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-class-api/__snapshots__/force-class-api.expected.routetypes.d.tsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-tags-api/__snapshots__/force-tags-api.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-tags-api/__snapshots__/force-tags-api.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-tags-api/__snapshots__/force-tags-api.expected.routetypes.d.tsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/multiple-sources/__snapshots__/multiple-sources.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/multiple-sources/__snapshots__/multiple-sources.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/nested-dynamic/__snapshots__/nested-dynamic.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/nested-dynamic/__snapshots__/nested-dynamic.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/optional-dynamic/__snapshots__/optional-dynamic.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/optional-dynamic/__snapshots__/optional-dynamic.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/optional-static/__snapshots__/optional-static.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/optional-static/__snapshots__/optional-static.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/optional-types/__snapshots__/optional-types.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/optional-types/__snapshots__/optional-types.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/optional-types/__snapshots__/optional-types.expected.routetypes.d.tsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/param-optional-rest/__snapshots__/param-optional-rest.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/param-optional-rest/__snapshots__/param-optional-rest.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/uri-encoded/__snapshots__/uri-encoded.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/uri-encoded/__snapshots__/uri-encoded.expected.routes.mdis excluded by!**/__snapshots__/**and included by**
📒 Files selected for processing (29)
.changeset/spicy-moles-stop.md(1 hunks).github/workflows/ci.yml(3 hunks)package.json(1 hunks)packages/run/package.json(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/.marko-run/routes.d.ts(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/marko.json(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/src/routes/+layout.marko(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/src/routes/+page.marko(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/test.config.ts(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/transform.ts(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/tsconfig.json(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/.marko-run/routes.d.ts(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/marko.json(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/src/routes/+layout.marko(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/src/routes/+page.marko(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/test.config.ts(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/transform.ts(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/tsconfig.json(1 hunks)packages/run/src/vite/__tests__/fixtures/build-routes/routes.txt(2 hunks)packages/run/src/vite/__tests__/fixtures/flat-routes/routes.txt(1 hunks)packages/run/src/vite/__tests__/fixtures/force-class-api/routes.txt(1 hunks)packages/run/src/vite/__tests__/fixtures/force-tags-api/routes.txt(1 hunks)packages/run/src/vite/__tests__/fixtures/optional-types/routes.txt(1 hunks)packages/run/src/vite/__tests__/main.test.ts(4 hunks)packages/run/src/vite/__tests__/route-builder.test.ts(1 hunks)packages/run/src/vite/__tests__/utils/fakeFS.ts(4 hunks)packages/run/src/vite/codegen/index.ts(3 hunks)packages/run/src/vite/plugin.ts(5 hunks)packages/run/src/vite/routes/walk.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
packages/run/src/__tests__/fixtures/force-class-api/test.config.ts (1)
packages/run/src/__tests__/fixtures/force-tags-api/test.config.ts (1)
steps(1-1)
packages/run/src/__tests__/fixtures/force-tags-api/test.config.ts (1)
packages/run/src/__tests__/fixtures/force-class-api/test.config.ts (1)
steps(1-1)
packages/run/src/vite/__tests__/main.test.ts (1)
packages/run/src/vite/__tests__/fixtures/build-routes/__snapshots__/build-routes.expected.routetypes.d.ts (15)
route(31-31)route(42-42)route(53-53)route(64-64)route(75-75)route(86-86)route(97-97)route(108-108)route(119-119)route(130-130)route(141-141)route(152-152)route(163-163)route(174-174)route(186-186)
packages/run/src/vite/__tests__/route-builder.test.ts (2)
packages/run/src/vite/routes/walk.ts (1)
createTestWalker(70-110)packages/run/src/vite/__tests__/utils/fakeFS.ts (1)
createDirectory(3-72)
packages/run/src/vite/codegen/index.ts (2)
packages/run/src/vite/codegen/writer.ts (1)
createStringWriter(165-179)packages/run/src/__tests__/fixtures/runtime-include/customAdapter.ts (1)
runtimeInclude(12-14)
packages/run/src/vite/plugin.ts (2)
packages/run/src/vite/types.ts (1)
Route(87-97)packages/run/src/vite/codegen/index.ts (1)
renderRouteTemplate(37-63)
packages/run/src/__tests__/fixtures/force-tags-api/transform.ts (1)
packages/run/src/__tests__/fixtures/force-class-api/transform.ts (1)
Program(4-39)
🔇 Additional comments (26)
packages/run/src/vite/__tests__/fixtures/flat-routes/routes.txt (1)
2-2: Verify route-fixture grammar + verb normalization (case/order).
Since this fixture now relies on an inline JSON payload ({ "verbs": ["get", "post"] }), please double-check the parser forroutes.txtaccepts the JSON in this position and whether it expects lowercase vs uppercase (and/or stable ordering) for verbs.packages/run/src/vite/__tests__/fixtures/force-tags-api/routes.txt (1)
1-2: Fixture looks consistent with “force tags API” intent (route-levelapi: "tags").packages/run/src/vite/__tests__/fixtures/optional-types/routes.txt (1)
3-3: Nice: explicit verbs metadata removes filename-coupling (handler.get.ts→handler.ts).packages/run/src/vite/__tests__/fixtures/build-routes/routes.txt (1)
4-4: Fixture updates correctly cover multi-verb and single-verb cases via{ "verbs": [...] }.Also applies to: 10-10, 12-12, 21-21, 23-23, 26-26
packages/run/src/__tests__/fixtures/force-tags-api/marko.json (1)
1-3: Good: fixturemarko.jsoncleanly wires the local transform.packages/run/src/vite/__tests__/fixtures/force-class-api/routes.txt (1)
1-2: Good symmetry with force-tags-api: route-levelapi: "class"coverage added.packages/run/src/__tests__/fixtures/force-tags-api/test.config.ts (1)
1-1: Fixture config matches the harness pattern.packages/run/src/__tests__/fixtures/force-class-api/test.config.ts (1)
1-1: Keeps force-class-api fixture consistent with force-tags-api.packages/run/src/__tests__/fixtures/force-class-api/src/routes/+page.marko (1)
1-1: Brittleness check:typeof component === "object"as the mode detector. If Marko’s runtime shapes change (or both modes yield an object), this fixture could become ambiguous—worth confirming against the Marko version under test.packages/run/src/__tests__/fixtures/force-class-api/marko.json (1)
1-3: Verify Marko can load a TypeScript transform viamarko.json. If the Marko toolchain here doesn’t natively load.ts, this will fail at compile time (and may need a.jswrapper or loader).packages/run/src/__tests__/fixtures/force-tags-api/src/routes/+layout.marko (1)
1-14: Verify layout slot rendering:<${input.content}/>andcomponentmode check. This is fine as a fixture, but it assumesinput.contentis present and renderable via dynamic-tag syntax, and thattypeof componentcleanly distinguishes class vs tags across the Marko version in use.packages/run/src/__tests__/fixtures/force-tags-api/transform.ts (1)
1-40: Regex lookbehind + Marko AST API usage: verify runtime/tooling compatibility.
(?<!entry)requires lookbehind support in the JS runtime running this transform; please confirm your supported Node range (not just CI).- Also worth confirming
t.markoPlaceholder(...)/t.templateLiteral(...)signatures are stable for the@marko/compilerversion in this repo.packages/run/src/__tests__/fixtures/force-class-api/.marko-run/routes.d.ts (1)
1-39: Generated typings: please verify module-specifier strings match the fixture’s actual import specifiers.If the compiler/runtime imports the routes using a different specifier (e.g., with an extension, different relative base, or virtual ids), these augmentations won’t apply and type coverage will silently degrade.
packages/run/src/vite/__tests__/route-builder.test.ts (1)
10-16: LGTM:createTestWalkernow takesTestFileTree, and this test passes.fileTreeaccordingly..changeset/spicy-moles-stop.md (1)
1-5: LGTM!The changeset correctly documents the patch-level change for @marko/run with a clear description that matches the PR objective.
.github/workflows/ci.yml (3)
60-63: Verify permission requirements for release job.Explicit permissions for
id-token,contents, andpull-requestshave been added to the release job. This appears to support provenance publishing or OIDC workflows.Ensure these permissions align with your changesets/action@v1 publishing requirements. If you're enabling npm provenance, verify the configuration is complete.
22-22: Node.js 24 is available and in Active LTS status.Node.js 24 has been in Active LTS since October 28, 2025, with maintenance guaranteed through April 30, 2028. The version is stable and suitable for use in CI workflows.
18-22: All GitHub Actions and Node.js versions in this workflow are stable and released. actions/checkout@v6 was released November 20, 2025 (v6.0.1 as of December 2), actions/setup-node@v6 was released October 14, 2025 (v6.1.0 as of December 3), and Node.js 24 entered Active LTS on October 28, 2025. No action needed.packages/run/src/vite/plugin.ts (3)
597-597: LGTM: Cache invalidation strategy.Clearing
routeMarkoApiCacheon route changes ensures fresh API lookups after file modifications. This aligns with the broaderbuildVirtualFilesResultandrenderVirtualFilesResultinvalidation strategy.
693-693: Non-null assertion safety.The non-null assertion on
virtualFiles.get(id)!assumes the file exists after callingrenderVirtualFiles. This should be safe given thevirtualFiles.has(id)check in theresolveIdhook (line 679).
243-261: The code is correct as-is. The assumption aboutroute.layouts[0]is safe becausetemplateFilePathis only set whencurrentLayouts.size > 0during route construction (lines 187–188 and 237–240 in builder.ts). The conditionif (route.templateFilePath)on line 248 implicitly guarantees that the layouts array is non-empty.The remaining concerns about module metadata reliability and cache invalidation are valid design considerations but not code issues.
packages/run/src/vite/codegen/index.ts (3)
37-63: LGTM: Refactored rendering approach.The signature change from
renderRouteTemplate(route, rootDir: string)torenderRouteTemplate(route, markoApi?: string)correctly reflects the new architecture:
- Marko API comment: When
markoApiis provided, it's written as an HTML comment<!-- use ${markoApi} -->, which Marko can parse to determine the rendering mode.- String writer: Using
createStringWriter()with branching provides better control over code generation.- Entry template tags: The new
writeEntryTemplateTagapproach generates nested layout/page component tags instead of relying on path-based imports.This aligns with the PR objective to use the Marko API of the top layout for route entry templates.
792-792: LGTM: Simplified path handling.Using
route.path.pathdirectly instead of adecodePathhelper simplifies the type generation. Ensure thatroute.path.pathis always properly formatted for TypeScript string literal types.
280-280: Verify backward compatibility of optionalruntimeIncludeparameter.Ensure all call sites of the function containing the optional
runtimeIncludeparameter handle this change correctly, particularly in adapter implementations.package.json (1)
55-55: No action required. The marko version 5.38.2 exists and is the current stable release (marked as "latest" on npm). The upgrade from ^5.37.40 to ^5.38.2 is a standard minor version bump with no breaking changes indicated.packages/run/package.json (1)
36-36: No action needed—@marko/vite version bump is appropriate.The upgrade from ^5.3.3 to ^5.4.2 is a safe minor version bump that includes v5.4.0's compatibility with Vite's environments API, which aligns with the PR's objective to use Marko API patterns for route entry templates. No breaking changes are introduced.
packages/run/src/__tests__/fixtures/force-tags-api/src/routes/+page.marko
Show resolved
Hide resolved
rturnq
force-pushed
the
route-entry-marko-api
branch
from
7db9b9c to
35645d4
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/run/src/vite/plugin.ts (1)
575-616: Dev invalidation misses layout changes: Marko API can go staleYou reset
routeMarkoApiCacheonly when rebuild triggers, but layout file edits (likely where Marko API changes) do not hit the current “change” condition. That can leave generated entry templates with the wrong<!-- use ... -->.Suggested adjustment (include
RoutableFileTypes.Layoutin the “change” condition; optionally alsoPageif you decide to fall back when no layouts):(type === "change" && (routableFileType === RoutableFileTypes.Handler || routableFileType === RoutableFileTypes.Middleware || + routableFileType === RoutableFileTypes.Layout || filename === runtimeInclude)) ) { buildVirtualFilesResult = undefined; renderVirtualFilesResult = undefined; routeMarkoApiCache = undefined;packages/run/src/vite/codegen/index.ts (1)
37-62: Remove embedded newline from Marko "use" directiveThe line
writer.writeLines(\\n`)embeds a newline inside the template string. SincewriteLines()always appends\nafter each line (per writer.ts line 76), this creates a double newline:\n\n`. The test snapshots show the expected output should have only a single blank line after the directive.Remove the embedded
\nand rely onwriteLines()to add the single newline:if (markoApi) { - writer.writeLines(`<!-- use ${markoApi} -->\n`); + writer.writeLines(`<!-- use ${markoApi} -->`); }The subsequent
writer.writeLines("")call on line 50 correctly adds the blank line before imports.
🧹 Nitpick comments (1)
packages/run/src/__tests__/fixtures/force-tags-api/transform.ts (1)
10-37: Injection payload matchesforce-class-api/transform.ts—keep them in lockstep to prevent fixture drift.The AST construction looks correct (template elements count matches expressions, placeholder usage consistent). Given the near-identical code between the two fixtures, consider a tiny shared helper (if the fixture setup allows it) or add a brief comment noting it must stay in sync with
packages/run/src/__tests__/fixtures/force-class-api/transform.ts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (11)
package-lock.jsonis excluded by!**/package-lock.jsonand included by**packages/run/src/__tests__/fixtures/force-class-api/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/force-class-api/__snapshots__/preview.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/force-tags-api/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/force-tags-api/__snapshots__/preview.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-class-api/__snapshots__/force-class-api.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-class-api/__snapshots__/force-class-api.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-class-api/__snapshots__/force-class-api.expected.routetypes.d.tsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-tags-api/__snapshots__/force-tags-api.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-tags-api/__snapshots__/force-tags-api.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-tags-api/__snapshots__/force-tags-api.expected.routetypes.d.tsis excluded by!**/__snapshots__/**and included by**
📒 Files selected for processing (23)
.changeset/spicy-moles-stop.md(1 hunks).github/workflows/ci.yml(3 hunks)package.json(1 hunks)packages/run/package.json(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/.marko-run/routes.d.ts(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/marko.json(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/src/routes/+layout.marko(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/src/routes/+page.marko(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/test.config.ts(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/transform.ts(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/tsconfig.json(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/.marko-run/routes.d.ts(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/marko.json(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/src/routes/+layout.marko(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/src/routes/+page.marko(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/test.config.ts(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/transform.ts(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/tsconfig.json(1 hunks)packages/run/src/vite/__tests__/fixtures/force-class-api/routes.txt(1 hunks)packages/run/src/vite/__tests__/fixtures/force-tags-api/routes.txt(1 hunks)packages/run/src/vite/__tests__/main.test.ts(4 hunks)packages/run/src/vite/codegen/index.ts(3 hunks)packages/run/src/vite/plugin.ts(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/run/src/tests/fixtures/force-tags-api/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (15)
- .changeset/spicy-moles-stop.md
- packages/run/src/tests/fixtures/force-class-api/test.config.ts
- packages/run/src/tests/fixtures/force-tags-api/marko.json
- packages/run/src/vite/tests/main.test.ts
- packages/run/src/tests/fixtures/force-tags-api/test.config.ts
- .github/workflows/ci.yml
- packages/run/src/tests/fixtures/force-class-api/tsconfig.json
- packages/run/src/tests/fixtures/force-class-api/transform.ts
- packages/run/src/tests/fixtures/force-class-api/marko.json
- packages/run/src/tests/fixtures/force-tags-api/src/routes/+page.marko
- packages/run/src/tests/fixtures/force-class-api/src/routes/+layout.marko
- packages/run/src/vite/tests/fixtures/force-tags-api/routes.txt
- packages/run/src/tests/fixtures/force-tags-api/src/routes/+layout.marko
- packages/run/src/vite/tests/fixtures/force-class-api/routes.txt
- packages/run/package.json
🧰 Additional context used
🧬 Code graph analysis (4)
packages/run/src/__tests__/fixtures/force-class-api/.marko-run/routes.d.ts (1)
packages/run/src/__tests__/fixtures/force-tags-api/.marko-run/routes.d.ts (9)
Route(21-21)Route(33-33)Context(22-22)Context(34-34)Handler(23-23)Handler(35-35)route(25-25)route(37-37)Input(30-30)
packages/run/src/__tests__/fixtures/force-tags-api/.marko-run/routes.d.ts (1)
packages/run/src/__tests__/fixtures/force-class-api/.marko-run/routes.d.ts (9)
Route(21-21)Route(33-33)Context(22-22)Context(34-34)Handler(23-23)Handler(35-35)route(25-25)route(37-37)Input(30-30)
packages/run/src/vite/plugin.ts (2)
packages/run/src/vite/types.ts (1)
Route(87-97)packages/run/src/vite/codegen/index.ts (1)
renderRouteTemplate(37-63)
packages/run/src/__tests__/fixtures/force-tags-api/transform.ts (1)
packages/run/src/__tests__/fixtures/force-class-api/transform.ts (1)
Program(4-39)
🔇 Additional comments (8)
packages/run/src/__tests__/fixtures/force-tags-api/.marko-run/routes.d.ts (1)
1-39: LGTM! Well-formed generated type declarations.The TypeScript declarations are correctly structured for the Marko Run integration. The file properly:
- Warns against manual edits
- Augments the module types for route "/"
- Provides consistent type exports for both +page.marko and +layout.marko
- Includes appropriate deprecation notices with migration guidance
- Matches the structure of the force-class-api fixture
packages/run/src/__tests__/fixtures/force-class-api/.marko-run/routes.d.ts (1)
6-8: Verify TS baseline +@marko/run/namespacesymbol availability for this generated.d.ts
A couple of things to double-check for stability: (1) the deprecation guidance usessatisfies(TS ≥ 4.9), and (2) this.d.tsimports runtime symbols from@marko/run/namespace, so ensure the installed@marko/runversion exports all of these and that this generated fixture output isn’t accidentally published/consumed outside tests.Also applies to: 10-16, 18-39
packages/run/src/__tests__/fixtures/force-class-api/src/routes/+page.marko (1)
1-1: No changes needed. This is transformer-injected code that intentionally usestypeof component === "object"to distinguish between Marko's class and tags APIs at runtime. The type definition inruntime/types.tsconfirms this is a documented structural difference: class API providesrenderBodywhile tags API providescontent. The paired test fixtures (force-class-apiandforce-tags-api) with their snapshots verify this check produces correct output in both scenarios, demonstrating it's not a heuristic but a reliable reflection of the actual component shape difference.Likely an incorrect or invalid review comment.
packages/run/src/__tests__/fixtures/force-tags-api/transform.ts (1)
6-7: No action needed. The codebase tests against Node.js 20–24, where RegExp lookbehind has been fully supported for years (since v8.10.0 in 2018). The regex pattern is safe and refactoring to a lookahead-based alternative is unnecessary.packages/run/src/vite/codegen/index.ts (2)
277-295:renderRouteroptionalruntimeIncludeparam looks fine; ensure call sites passundefinedwhen absent
763-807: The path encoding concern here is unfounded. Theroute.path.pathvalue is already normalized throughnormalizeSegment(), which decodes URI components and then selectively re-encodes only the characters/?#. This normalized format is intentional and consistent with howmatch_internal()compares paths at runtime—both use the same normalized representation. The generated TS types correctly reflect the normalized path strings returned by the router, so there is no mismatch between static and runtime expectations.If anything, consider documenting that
Route.path.pathcontains normalized paths (not raw filesystem paths), but no code changes are needed here.Likely an incorrect or invalid review comment.
packages/run/src/vite/plugin.ts (2)
688-700: Non-null assertion aftervirtualFiles.has(id)is OK
292-323: Per-route API threaded into renderRouteTemplate: correct approachThis correctly passes a per-route
markoApiintorenderRouteTemplate(...)by retrieving it from module metadata. The pattern is sound:
- Dev path (
context.environment.transformRequest+context.getModuleInfo): Vite 7's per-environment API runs the full transform pipeline, accumulates metadata from plugin hooks, and stores it on the ModuleNode.getModuleInforeliably reflects that metadata aftertransformRequestcompletes.- Build path (
context.load): Also returns module metadata through the standard plugin context, with the same meta propagation from prior transform hooks.The metadata is populated by the
markoVitePlugin(which runs before this plugin) via itsload/transformhooks, then reliably retrieved in both dev and build via the approaches shown. The caching of results per-route is appropriate.
rturnq
force-pushed
the
route-entry-marko-api
branch
2 times, most recently
from
3e79ae0 to
743b5b1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
package.json (1)
53-53: marko@5.38.2 does not exist on npm and will cause installation failures.The latest published version of marko is 5.37.63. Update the version to a valid published release (e.g.,
^5.37.63) to prevent dependency resolution failures.Apply this diff:
- "marko": "^5.38.2", + "marko": "^5.37.63",
🧹 Nitpick comments (2)
packages/run/src/__tests__/fixtures/netlify-adapter-not-edge/test.config.ts (1)
9-9: Avoid unconditional skipping (or add a clear opt-in override + rationale).Hard-coding
skip_preview = truemeans this fixture can’t be exercised locally anymore. If this is intentional (e.g., flaky/slow), please add a short rationale comment (not commented-out code), and consider an escape hatch likeSKIP_PREVIEW !== "false"so developers can still run it when needed.packages/run/src/__tests__/fixtures/netlify-adapter-edge/test.config.ts (1)
9-9: Prefer clean rationale comment (and keep style consistent across fixtures).Inline commented-out logic is noisy; also consider matching the semicolon/style used in the other fixture to avoid lint/prettier churn.
-export const skip_preview = true //process.env.CI === "true"; +// Previews are intentionally disabled for this fixture (see PR context / flakiness / Netlify base-path changes). +export const skip_preview = true;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.jsonand included by**
📒 Files selected for processing (7)
package.json(1 hunks)packages/run/src/__tests__/fixtures/netlify-adapter-edge/netlify.toml(1 hunks)packages/run/src/__tests__/fixtures/netlify-adapter-edge/test.config.ts(1 hunks)packages/run/src/__tests__/fixtures/netlify-adapter-not-edge/test.config.ts(1 hunks)packages/run/src/__tests__/main.test.ts(1 hunks)packages/run/src/vite/__tests__/main.test.ts(5 hunks)packages/run/src/vite/utils/log.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/run/src/vite/utils/log.ts
🧰 Additional context used
🧬 Code graph analysis (2)
packages/run/src/__tests__/fixtures/netlify-adapter-not-edge/test.config.ts (1)
packages/run/src/__tests__/fixtures/netlify-adapter-edge/test.config.ts (1)
skip_preview(9-9)
packages/run/src/vite/__tests__/main.test.ts (5)
packages/run/src/vite/__tests__/utils/fakeFS.ts (1)
createDirectory(3-72)packages/run/src/vite/routes/walk.ts (1)
createTestWalker(70-110)packages/run/src/vite/constants.ts (2)
HttpVerb(29-29)httpVerbs(8-16)packages/run/src/vite/index.ts (1)
HttpVerb(8-8)packages/run/src/vite/codegen/index.ts (1)
renderRouteTemplate(37-63)
🔇 Additional comments (5)
packages/run/src/__tests__/main.test.ts (1)
5-5: LGTM!Clean simplification removing the CommonJS/ESM interop workaround. The direct default import is the preferred approach.
packages/run/src/vite/__tests__/main.test.ts (4)
2-2: LGTM!Same clean import simplification as in the other test file.
38-55: LGTM!Good refactor to capture the full result from
createDirectory, enabling access to both the file tree structure and associated metadata. The optional typing correctly handles the case where no routes file is found.
100-105: LGTM!The verb extraction logic correctly validates against known HTTP verbs and provides a sensible fallback. The
as anycast is a reasonable type workaround for the readonly-to-mutable array assignment in test code.
129-137: LGTM!Special routes correctly check
route.layouts.lengthbefore accessing layouts, and appropriately omit themarkoApiparameter.
rturnq
force-pushed
the
route-entry-marko-api
branch
from
743b5b1 to
6db9215
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/run/src/vite/__tests__/main.test.ts (1)
112-118: Add defensive check for empty layouts array.Accessing
route.layouts[0]without verifying the array has elements could throw iflayoutsis empty. The special routes section (lines 129-130) explicitly checksroute.layouts.lengthbefore accessing layouts.Consider adding a guard:
- const layoutMeta = testFs?.meta.get(route.layouts[0].filePath) as + const layoutMeta = route.layouts.length + ? testFs?.meta.get(route.layouts[0].filePath) as | { api: string } - | undefined; + | undefined + : undefined;Alternatively, verify that
templateFilePathbeing set guarantees non-emptylayouts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.jsonand included by**
📒 Files selected for processing (7)
package.json(1 hunks)packages/run/src/__tests__/fixtures/netlify-adapter-edge/netlify.toml(1 hunks)packages/run/src/__tests__/fixtures/netlify-adapter-edge/test.config.ts(1 hunks)packages/run/src/__tests__/fixtures/netlify-adapter-not-edge/test.config.ts(1 hunks)packages/run/src/__tests__/main.test.ts(1 hunks)packages/run/src/vite/__tests__/main.test.ts(5 hunks)packages/run/src/vite/utils/log.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- package.json
- packages/run/src/vite/utils/log.ts
- packages/run/src/tests/fixtures/netlify-adapter-edge/netlify.toml
🧰 Additional context used
🧬 Code graph analysis (2)
packages/run/src/__tests__/fixtures/netlify-adapter-not-edge/test.config.ts (1)
packages/run/src/__tests__/fixtures/netlify-adapter-edge/test.config.ts (1)
skip_preview(9-9)
packages/run/src/__tests__/fixtures/netlify-adapter-edge/test.config.ts (1)
packages/run/src/__tests__/fixtures/netlify-adapter-not-edge/test.config.ts (1)
skip_preview(9-9)
🔇 Additional comments (6)
packages/run/src/vite/__tests__/main.test.ts (5)
2-2: LGTM!The import statement has been updated to use a named import, which is consistent with modern ES module practices.
38-38: LGTM!The explicit type annotation for
testFsimproves type safety and makes the code more maintainable.
51-53: LGTM!The changes align with the new testFs-based filesystem abstraction, properly creating the test filesystem and passing
fileTreeto the walker.
100-105: LGTM!The handler metadata extraction correctly retrieves verb information from the test filesystem metadata and appropriately filters to valid HTTP verbs with a sensible fallback.
134-134: LGTM!The simplified call to
renderRouteTemplatefor special routes is appropriate and aligns with the updated function signature.packages/run/src/__tests__/main.test.ts (1)
5-5: The default import is correct and safe. mocha-snap's official documentation recommendsimport snap from "mocha-snap";for ESM, which is exactly what the code uses. The TypeScript configuration includesesModuleInteropandallowSyntheticDefaultImportsenabled, and the package is marked as ESM ("type": "module"). No fallback pattern or workaround is needed.
packages/run/src/__tests__/fixtures/netlify-adapter-not-edge/test.config.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/run/src/__tests__/fixtures/netlify-adapter-edge/netlify.toml (1)
2-4: Makebuild.basefixture-relative (current value is likely wrong whencwdis the fixture).
Setbase = "."or removebasesopublishandedge_functionsresolve relative to the fixture directory; the current repo-relative string can break when the Netlify CLI is invoked withcwdset to this fixture.[build] -base = "packages/run/src/__tests__/fixtures/netlify-adapter-edge" +base = "." publish = "dist/public" edge_functions = "dist"packages/run/src/vite/__tests__/main.test.ts (1)
112-114: Add defensive check for empty layouts array.Accessing
route.layouts[0].filePathwithout verifying the array is non-empty could throw a runtime error. The special routes section (lines 129-130) explicitly checksroute.layouts.lengthbefore accessing layouts, which indicates this is a real concern.As suggested in the previous review, consider adding a guard:
- const layoutMeta = testFs?.meta.get(route.layouts[0].filePath) as + const layoutMeta = route.layouts.length + ? testFs?.meta.get(route.layouts[0].filePath) as | { api: string } - | undefined; + | undefined + : undefined;Alternatively, if
route.templateFilePathbeing set guarantees non-emptylayouts, this should be verified and documented.
🧹 Nitpick comments (1)
packages/run/src/vite/__tests__/main.test.ts (1)
100-105: Consider clarifying the fallback behavior for empty verbs.When
meta.verbsis an empty array (or contains only invalid verbs), the logic falls back to allhttpVerbs. This might be intentional, but it's worth confirming whether an explicitly empty verbs array should mean "no verbs" or "all verbs."If the current behavior is correct, consider adding a brief comment:
const meta = testFs?.meta.get(route.handler.filePath) as | { verbs: HttpVerb[] } | undefined; const verbs = meta?.verbs.filter((v) => httpVerbs.includes(v)); +// Fallback to all verbs when no valid verbs are specified route.handler.verbs = verbs?.length ? verbs : (httpVerbs as any);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.jsonand included by**
📒 Files selected for processing (7)
package.json(1 hunks)packages/run/src/__tests__/fixtures/netlify-adapter-edge/netlify.toml(1 hunks)packages/run/src/__tests__/fixtures/netlify-adapter-edge/test.config.ts(1 hunks)packages/run/src/__tests__/fixtures/netlify-adapter-not-edge/test.config.ts(1 hunks)packages/run/src/__tests__/main.test.ts(1 hunks)packages/run/src/vite/__tests__/main.test.ts(5 hunks)packages/run/src/vite/utils/log.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- package.json
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/run/src/tests/fixtures/netlify-adapter-edge/test.config.ts
- packages/run/src/tests/main.test.ts
- packages/run/src/tests/fixtures/netlify-adapter-not-edge/test.config.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/run/src/vite/__tests__/main.test.ts (4)
packages/run/src/vite/__tests__/utils/fakeFS.ts (1)
createDirectory(3-72)packages/run/src/vite/routes/walk.ts (1)
createTestWalker(70-110)packages/run/src/vite/constants.ts (2)
HttpVerb(29-29)httpVerbs(8-16)packages/run/src/vite/codegen/index.ts (1)
renderRouteTemplate(37-63)
🔇 Additional comments (2)
packages/run/src/vite/__tests__/main.test.ts (2)
2-2: LGTM: Clean integration of the new fakeFS API.The refactoring to use the new
createDirectoryAPI that returns{fileTree, meta}is well-structured and properly typed.Also applies to: 38-38, 51-53
134-134: LGTM: Correct usage of optional parameter.The call to
renderRouteTemplate(route)without themarkoApiparameter is correct, as the parameter is optional and special routes don't require layout API metadata.
rturnq
force-pushed
the
route-entry-marko-api
branch
from
6db9215 to
7c1fe08
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
packages/run/src/__tests__/fixtures/netlify-adapter-edge/test.config.ts (1)
9-9: Re-add the semicolon + clarify whether unconditional preview skip is intended (regression vs prior fix).
Right now this differs fromnetlify-adapter-not-edge(and reads like a partially-reverted conditional). Either (a) make it conditional again, or (b) keep it unconditional but add the semicolon and a clear comment for why preview is always skipped so it doesn’t hide coverage gaps.-export const skip_preview = true //process.env.CI === "true"; +export const skip_preview = true; // Intentionally always skipped: <reason> +// Alternatively: +// export const skip_preview = process.env.CI === "true";packages/run/src/__tests__/fixtures/netlify-adapter-edge/netlify.toml (1)
1-4: Avoid brittle repo-relativebasein fixture; prefer.(or omit).
The currentbasevalue ties the fixture to a specific repo layout/cwd. Usingbase = "."(or removingbase) makespublish/edge_functionsresolve relative to the fixture directory and is more robust.[build] -base = "packages/run/src/__tests__/fixtures/netlify-adapter-edge" +base = "." publish = "dist/public" edge_functions = "dist"packages/run/src/vite/__tests__/main.test.ts (1)
112-118: Guardroute.layouts[0]access (still can throw).This is the same issue called out in the prior review: template routes don’t necessarily guarantee
route.layouts.length > 0, but the code unconditionally readsroute.layouts[0].filePath.- const layoutMeta = testFs?.meta.get(route.layouts[0].filePath) as - | { api: string } - | undefined; + const layoutMeta = route.layouts.length + ? (testFs?.meta.get(route.layouts[0].filePath) as + | { api: string } + | undefined) + : undefined;
🧹 Nitpick comments (3)
packages/run/src/vite/__tests__/main.test.ts (3)
38-55:testFsis overwritten if multipleroutes*.txtexist.
If a fixture ever includes more than oneroutes(-x).txt, the last one wins for all metadata lookups, which can silently skew verbs/Marko API expectations for routes coming from earlier sources. Consider either asserting only one match or trackingmetaper source.
100-105: Avoidanyand hardenmeta.verbsaccess.
This keeps the test harness resilient to partial metadata and removes theas anyescape hatch.- const verbs = meta?.verbs.filter((v) => httpVerbs.includes(v)); - route.handler.verbs = verbs?.length ? verbs : (httpVerbs as any); + const verbs = meta?.verbs?.filter((v) => httpVerbs.includes(v)); + route.handler.verbs = verbs?.length ? verbs : [...httpVerbs];
112-115: Potential meta lookup misses on Windows path separators.
createDirectory()stores meta keys usingdirStack.join("/"), whileroute.*.filePathmay contain\depending on how routes are built. If you want the “pseudo test on windows” comment (Line 49) to remain viable, consider normalizing the lookup key(s) beforemeta.get(...)(e.g., replace\\with/, or centralize a helper).Also applies to: 100-103
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.jsonand included by**
📒 Files selected for processing (7)
package.json(1 hunks)packages/run/src/__tests__/fixtures/netlify-adapter-edge/netlify.toml(1 hunks)packages/run/src/__tests__/fixtures/netlify-adapter-edge/test.config.ts(1 hunks)packages/run/src/__tests__/fixtures/netlify-adapter-not-edge/test.config.ts(1 hunks)packages/run/src/__tests__/main.test.ts(1 hunks)packages/run/src/vite/__tests__/main.test.ts(5 hunks)packages/run/src/vite/utils/log.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/run/src/tests/main.test.ts
- package.json
- packages/run/src/tests/fixtures/netlify-adapter-not-edge/test.config.ts
- packages/run/src/vite/utils/log.ts
🧰 Additional context used
🧬 Code graph analysis (2)
packages/run/src/__tests__/fixtures/netlify-adapter-edge/test.config.ts (1)
packages/run/src/__tests__/fixtures/netlify-adapter-not-edge/test.config.ts (1)
skip_preview(9-9)
packages/run/src/vite/__tests__/main.test.ts (4)
packages/run/src/vite/__tests__/utils/fakeFS.ts (1)
createDirectory(3-72)packages/run/src/vite/routes/walk.ts (1)
createTestWalker(70-110)packages/run/src/vite/constants.ts (2)
HttpVerb(29-29)httpVerbs(8-16)packages/run/src/vite/codegen/index.ts (1)
renderRouteTemplate(37-63)
🔇 Additional comments (2)
packages/run/src/vite/__tests__/main.test.ts (2)
2-2: Import alias rename looks fine.
Keeps the callsites concise/consistent with typical “snapshot” naming.
129-136: Special routes omit Marko API injection—confirm intent.
You now render special route templates without passingmarkoApi(Line 134-135) even though you requireroute.layouts.length(Line 130). If special routes should also reflect the “top layout Marko API” behavior, they likely need the same metadata path.
rturnq
force-pushed
the
route-entry-marko-api
branch
2 times, most recently
from
4b208ca to
2e9388c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/run/src/vite/__tests__/main.test.ts (1)
38-55:testFsgets overwritten when multipleroutes*.txtfiles exist, causing metadata lookups to fail for earlier sources.When a fixture contains multiple
routes(-x).txtfiles, the loop creates multiple entries insources(each with its own walker), buttestFsis overwritten on each iteration and only retains the last directory's{ fileTree, meta }. Later lookups at lines 67 and 83 (testFs?.meta.get(route.handler.filePath)) will only succeed for routes from the final source, causing undefined metadata for routes from earlier sources.Consider either:
- enforcing exactly one
routes*.txtper fixture (throw if more than one), or- maintaining a map of metadata keyed by source (matching the
sourcesarray structure).Example "enforce one" diff:
- let testFs: ReturnType<typeof createDirectory> | undefined; + let testFs: ReturnType<typeof createDirectory> | undefined; + let routesFileCount = 0; ... testFs = createDirectory(src, routesDir); + routesFileCount++; + if (routesFileCount > 1) { + throw new Error("Only one routes*.txt is supported per fixture (testFs/meta would be ambiguous)."); + } sources.push({ walker: createTestWalker(testFs.fileTree), basePath: match[1], });
♻️ Duplicate comments (6)
packages/run/src/__tests__/fixtures/netlify-adapter-edge/netlify.toml (1)
2-2: Absolutebasepath should be relative (duplicate concern from prior review).A previous review flagged that the
basefield uses an absolute repo path, which may not resolve correctly when the Netlify CLI is invoked from the fixture directory. Use a relative path (e.g.,base = ".") or omit the field entirely so thatpublishandedge_functionsare resolved relative to the fixturecwd. While this is currently moot due toskip_preview = truein the test config, it should be corrected for future compatibility.packages/run/src/vite/__tests__/main.test.ts (1)
112-118: Guardroute.layouts[0]access (can throw when layouts is empty).This is the same concern as the prior review:
route.templateFilePathdoesn’t obviously guaranteeroute.layouts.length > 0, and special routes already defensively checkroute.layouts.length.- const layoutMeta = testFs?.meta.get(route.layouts[0].filePath) as - | { api: string } - | undefined; + const layoutMeta = + route.layouts.length > 0 + ? (testFs?.meta.get(route.layouts[0].filePath) as + | { api: string } + | undefined) + : undefined; ... - routesSnap += renderRouteTemplate(route, layoutMeta?.api); + routesSnap += renderRouteTemplate(route, layoutMeta?.api);examples/node-express/package.json (1)
14-14: marko@5.38.2 does not exist on npm and will cause installation failures.The latest published version of marko is 5.37.63. Update this to a valid published version to prevent dependency resolution failures during installation.
- "marko": "^5.38.2" + "marko": "^5.37.63"examples/static/package.json (1)
12-12: marko@5.38.2 does not exist on npm and will cause installation failures.The latest published version of marko is 5.37.63. Update this to a valid published version to prevent dependency resolution failures during installation.
- "marko": "^5.38.2" + "marko": "^5.37.63"examples/netlify/package.json (1)
12-12: marko@5.38.2 does not exist on npm and will cause installation failures.The latest published version of marko is 5.37.63. Update this to a valid published version to prevent dependency resolution failures during installation.
- "marko": "^5.38.2" + "marko": "^5.37.63"package.json (1)
52-52: marko@5.38.2 does not exist on npm and will cause installation failures.The latest published version of marko is 5.37.63. Update this to a valid published version to prevent dependency resolution failures during installation.
- "marko": "^5.38.2", + "marko": "^5.37.63",
🧹 Nitpick comments (1)
packages/run/src/vite/__tests__/main.test.ts (1)
100-105: Prefer a typed fallback instead of(httpVerbs as any)to keep tests honest.
(httpVerbs as any)can mask type regressions. If you need a fallback, keep it typed:- route.handler.verbs = verbs?.length ? verbs : (httpVerbs as any); + route.handler.verbs = verbs?.length ? verbs : (httpVerbs as HttpVerb[]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.jsonand included by**
📒 Files selected for processing (11)
examples/netlify/package.json(1 hunks)examples/node-express/package.json(1 hunks)examples/static/package.json(1 hunks)examples/zero-config/package.json(1 hunks)package.json(1 hunks)packages/run/src/__tests__/fixtures/netlify-adapter-edge/netlify.toml(1 hunks)packages/run/src/__tests__/fixtures/netlify-adapter-edge/test.config.ts(1 hunks)packages/run/src/__tests__/fixtures/netlify-adapter-not-edge/test.config.ts(1 hunks)packages/run/src/__tests__/main.test.ts(1 hunks)packages/run/src/vite/__tests__/main.test.ts(5 hunks)packages/run/src/vite/utils/log.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/run/src/vite/utils/log.ts
- packages/run/src/tests/main.test.ts
- packages/run/src/tests/fixtures/netlify-adapter-edge/test.config.ts
- packages/run/src/tests/fixtures/netlify-adapter-not-edge/test.config.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test: node@20 (ubuntu-latest)
- GitHub Check: test: node@24 (ubuntu-latest)
- GitHub Check: test: node@22 (windows-latest)
- GitHub Check: test: node@24 (windows-latest)
- GitHub Check: test: node@20 (windows-latest)
- GitHub Check: test: node@22 (ubuntu-latest)
🔇 Additional comments (2)
packages/run/src/vite/__tests__/main.test.ts (2)
2-2: Verifymocha-snapdefault import is valid in this test environment.If
mocha-snapdoesn’t provide a default export under your TS/ESM settings, this will fail at runtime; ensure this matches how the package is exported/resolved in the repo.
129-135: Special routes: confirm whether omittinglayoutMeta?.apiis intentional (consistency/determinism).For normal routes you pass
layoutMeta?.apiintorenderRouteTemplate(route, ...), but for special routes you don’t. IfrenderRouteTemplatenow infers per-route Marko API internally, consider removing the normal-route param too; otherwise, fetch/pass the same layout api here for consistent snapshots.
rturnq
force-pushed
the
route-entry-marko-api
branch
from
2e9388c to
726a9f0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
packages/run/src/__tests__/fixtures/netlify-adapter-not-edge/test.config.ts (1)
9-9: The hard-codedskip_preview = trueissue persists.This line was previously flagged for permanently disabling preview tests. The commented-out conditional suggests this should be environment-based rather than hard-coded.
packages/run/src/__tests__/fixtures/netlify-adapter-edge/test.config.ts (1)
9-9: Missing semicolon creates style inconsistency with related fixture.The semicolon after
trueis missing, creating a style inconsistency with thenetlify-adapter-not-edgefixture. The past review requesting this fix was marked as addressed, but the semicolon is still absent.Apply this diff to align the style:
-export const skip_preview = true //process.env.CI === "true"; +export const skip_preview = true; //process.env.CI === "true";examples/zero-config/package.json (1)
12-12: Confirmmarko@^5.38.2is actually published (otherwise installs break).Is the npm package "marko" published with a version 5.38.2? If not, what is the latest 5.x version available?package.json (1)
26-28: Please verifymarko@^5.38.2exists on npm; otherwise installs will fail.Is the npm package "marko" published with a version 5.38.2? If not, what is the latest 5.x version available?Also applies to: 30-31, 34-35, 37-38, 41-45, 48-52, 54-65
packages/run/src/vite/plugin.ts (1)
243-261: FixgetMarkoApiForRoute: cache miss onundefined+ possible crash whenroute.layoutsis empty.let routeMarkoApiCache: Map<Route, string | undefined> | undefined; async function getMarkoApiForRoute(route: Route, context: PluginContext) { routeMarkoApiCache ??= new Map(); - let markoAPI = routeMarkoApiCache.get(route); - if (!markoAPI) { - if (route.templateFilePath) { - const layoutFilePath = route.layouts[0].filePath; - const module = - "transformRequest" in context.environment - ? (await context.environment.transformRequest(layoutFilePath), - context.getModuleInfo(layoutFilePath)) - : await context.load({ id: layoutFilePath }); - markoAPI = module?.meta?.markoAPI; - } - - routeMarkoApiCache.set(route, markoAPI); - } - return markoAPI; + if (routeMarkoApiCache.has(route)) return routeMarkoApiCache.get(route); + + let markoAPI: string | undefined; + if (route.templateFilePath) { + const topLayout = route.layouts[0]; + if (topLayout) { + const layoutFilePath = topLayout.filePath; + const module = + "transformRequest" in context.environment + ? (await context.environment.transformRequest(layoutFilePath), + context.getModuleInfo(layoutFilePath)) + : await context.load({ id: layoutFilePath }); + markoAPI = (module as any)?.meta?.markoAPI; + } + } + + routeMarkoApiCache.set(route, markoAPI); + return markoAPI; }packages/run/src/vite/__tests__/main.test.ts (1)
119-125: Guardroute.layouts[0]access before reading layout meta.
Templates can plausibly exist without a layout; this will throw during snapshot generation. (Matches prior review feedback.)- const layoutData = getFileData<{ api: string }>(route.layouts[0]); + const layoutData = route.layouts.length + ? getFileData<{ api: string }>(route.layouts[0]) + : undefined;packages/run/src/vite/__tests__/utils/fakeFS.ts (1)
3-9: Meta key construction is not cross-platform (hard-coded/).
This will break lookups on Windows / whenbaseDircontains\(and consumers callnormalizePath(...)). (Matches prior review feedback.)+import path from "path"; import type { TestFileTree } from "../../routes/walk"; @@ - meta.set( - `${dirStack.join("/")}/${name}`, - JSON.parse(match[1].slice(1)), - ); + const key = path.join(...dirStack, name); + meta.set(key, JSON.parse(match[1].slice(1)));Also applies to: 13-16, 49-59, 68-71
🧹 Nitpick comments (2)
packages/run/src/vite/__tests__/main.test.ts (1)
107-114: Good: fixture-driven verbs + filtering tohttpVerbs.
Minor: considerroute.handler.verbs = verbs?.length ? verbs : [...httpVerbs]to avoid sharing the readonly tuple instance if anything later mutates it.packages/run/src/vite/__tests__/utils/fakeFS.ts (1)
10-66: Optional: improve JSON meta parse errors with line context.
WhenJSON.parse(...)fails, includinglineNoand the raw meta substring will make fixture debugging faster.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (38)
package-lock.jsonis excluded by!**/package-lock.jsonand included by**packages/run/src/__tests__/fixtures/force-class-api/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/force-class-api/__snapshots__/preview.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/force-tags-api/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/force-tags-api/__snapshots__/preview.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/basic-500/__snapshots__/basic-500.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/basic-500/__snapshots__/basic-500.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/basic/__snapshots__/basic.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/basic/__snapshots__/basic.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/build-routes/__snapshots__/build-routes.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/build-routes/__snapshots__/build-routes.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/build-routes/__snapshots__/build-routes.expected.routetypes.d.tsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/escape-path/__snapshots__/escape-path.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/escape-path/__snapshots__/escape-path.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/flat-routes/__snapshots__/flat-routes.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/flat-routes/__snapshots__/flat-routes.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/flat-routes/__snapshots__/flat-routes.expected.routetypes.d.tsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-class-api/__snapshots__/force-class-api.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-class-api/__snapshots__/force-class-api.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-class-api/__snapshots__/force-class-api.expected.routetypes.d.tsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-tags-api/__snapshots__/force-tags-api.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-tags-api/__snapshots__/force-tags-api.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-tags-api/__snapshots__/force-tags-api.expected.routetypes.d.tsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/multiple-sources/__snapshots__/multiple-sources.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/multiple-sources/__snapshots__/multiple-sources.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/nested-dynamic/__snapshots__/nested-dynamic.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/nested-dynamic/__snapshots__/nested-dynamic.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/optional-dynamic/__snapshots__/optional-dynamic.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/optional-dynamic/__snapshots__/optional-dynamic.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/optional-static/__snapshots__/optional-static.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/optional-static/__snapshots__/optional-static.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/optional-types/__snapshots__/optional-types.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/optional-types/__snapshots__/optional-types.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/optional-types/__snapshots__/optional-types.expected.routetypes.d.tsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/param-optional-rest/__snapshots__/param-optional-rest.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/param-optional-rest/__snapshots__/param-optional-rest.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/uri-encoded/__snapshots__/uri-encoded.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/uri-encoded/__snapshots__/uri-encoded.expected.routes.mdis excluded by!**/__snapshots__/**and included by**
📒 Files selected for processing (39)
.changeset/spicy-moles-stop.md(1 hunks).github/workflows/ci.yml(3 hunks)examples/netlify/package.json(1 hunks)examples/node-express/package.json(1 hunks)examples/static/package.json(1 hunks)examples/zero-config/package.json(1 hunks)package.json(1 hunks)packages/explorer/package.json(1 hunks)packages/run/package.json(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/.marko-run/routes.d.ts(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/marko.json(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/src/routes/+layout.marko(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/src/routes/+page.marko(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/test.config.ts(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/transform.ts(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/tsconfig.json(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/.marko-run/routes.d.ts(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/marko.json(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/src/routes/+layout.marko(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/src/routes/+page.marko(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/test.config.ts(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/transform.ts(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/tsconfig.json(1 hunks)packages/run/src/__tests__/fixtures/netlify-adapter-edge/netlify.toml(1 hunks)packages/run/src/__tests__/fixtures/netlify-adapter-edge/test.config.ts(1 hunks)packages/run/src/__tests__/fixtures/netlify-adapter-not-edge/test.config.ts(1 hunks)packages/run/src/__tests__/main.test.ts(1 hunks)packages/run/src/vite/__tests__/fixtures/build-routes/routes.txt(2 hunks)packages/run/src/vite/__tests__/fixtures/flat-routes/routes.txt(1 hunks)packages/run/src/vite/__tests__/fixtures/force-class-api/routes.txt(1 hunks)packages/run/src/vite/__tests__/fixtures/force-tags-api/routes.txt(1 hunks)packages/run/src/vite/__tests__/fixtures/optional-types/routes.txt(1 hunks)packages/run/src/vite/__tests__/main.test.ts(6 hunks)packages/run/src/vite/__tests__/route-builder.test.ts(1 hunks)packages/run/src/vite/__tests__/utils/fakeFS.ts(4 hunks)packages/run/src/vite/codegen/index.ts(3 hunks)packages/run/src/vite/plugin.ts(5 hunks)packages/run/src/vite/routes/walk.ts(2 hunks)packages/run/src/vite/utils/log.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/run/src/tests/fixtures/force-tags-api/marko.json
🚧 Files skipped from review as they are similar to previous changes (19)
- packages/run/package.json
- packages/run/src/tests/fixtures/force-class-api/src/routes/+layout.marko
- packages/run/src/tests/fixtures/force-class-api/tsconfig.json
- packages/run/src/vite/tests/fixtures/force-tags-api/routes.txt
- packages/run/src/tests/fixtures/force-class-api/test.config.ts
- examples/netlify/package.json
- packages/run/src/tests/fixtures/force-tags-api/tsconfig.json
- packages/run/src/vite/utils/log.ts
- packages/run/src/tests/main.test.ts
- packages/run/src/tests/fixtures/force-tags-api/transform.ts
- examples/node-express/package.json
- packages/run/src/tests/fixtures/force-tags-api/.marko-run/routes.d.ts
- packages/run/src/tests/fixtures/netlify-adapter-edge/netlify.toml
- examples/static/package.json
- packages/run/src/tests/fixtures/force-tags-api/test.config.ts
- packages/run/src/tests/fixtures/force-class-api/transform.ts
- .changeset/spicy-moles-stop.md
- packages/run/src/tests/fixtures/force-tags-api/src/routes/+page.marko
- packages/run/src/vite/tests/fixtures/force-class-api/routes.txt
🧰 Additional context used
🧬 Code graph analysis (7)
packages/run/src/vite/__tests__/main.test.ts (6)
packages/run/src/vite/types.ts (2)
RoutableFile(112-118)HttpVerb(11-11)packages/run/src/vite/__tests__/utils/fakeFS.ts (1)
createDirectory(3-72)packages/run/src/vite/routes/walk.ts (1)
createTestWalker(70-110)packages/run/src/vite/utils/fs.ts (1)
normalizePath(5-8)packages/run/src/vite/constants.ts (2)
HttpVerb(29-29)httpVerbs(8-16)packages/run/src/vite/codegen/index.ts (1)
renderRouteTemplate(37-63)
packages/run/src/vite/__tests__/utils/fakeFS.ts (1)
packages/run/src/vite/routes/walk.ts (1)
TestFileTree(68-68)
packages/run/src/vite/codegen/index.ts (1)
packages/run/src/vite/codegen/writer.ts (1)
createStringWriter(165-179)
packages/run/src/vite/__tests__/route-builder.test.ts (2)
packages/run/src/vite/routes/walk.ts (1)
createTestWalker(70-110)packages/run/src/vite/__tests__/utils/fakeFS.ts (1)
createDirectory(3-72)
packages/run/src/__tests__/fixtures/netlify-adapter-not-edge/test.config.ts (1)
packages/run/src/__tests__/fixtures/netlify-adapter-edge/test.config.ts (1)
skip_preview(9-9)
packages/run/src/vite/plugin.ts (2)
packages/run/src/vite/types.ts (1)
Route(87-97)packages/run/src/vite/codegen/index.ts (1)
renderRouteTemplate(37-63)
packages/run/src/__tests__/fixtures/netlify-adapter-edge/test.config.ts (1)
packages/run/src/__tests__/fixtures/netlify-adapter-not-edge/test.config.ts (1)
skip_preview(9-9)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test: node@24 (windows-latest)
- GitHub Check: test: node@20 (windows-latest)
- GitHub Check: test: node@24 (ubuntu-latest)
- GitHub Check: test: node@22 (ubuntu-latest)
- GitHub Check: test: node@20 (ubuntu-latest)
- GitHub Check: test: node@22 (windows-latest)
🔇 Additional comments (17)
packages/run/src/__tests__/fixtures/force-class-api/marko.json (1)
1-3: Verify Marko can load a TypeScript transform here (vs requiring compiled JS).
If the transform loader doesn’t handle.tsin this fixture environment, tests will fail at runtime resolving./transform.ts.Marko compiler transform config: does "transform" support a TypeScript file path (e.g., "./transform.ts") directly, and what loader/runtime requirements (ts-node/register, ESM hooks) are needed?packages/run/src/__tests__/fixtures/force-class-api/.marko-run/routes.d.ts (1)
6-39: This generated fixture file follows the established pattern used across 50+ test fixtures in the repository. TheMarko.Globalreference and self-referentialtypeof import()pattern are standard and working—they're consistently applied throughout the codebase and have been validated in CI/dev environments. No action needed.packages/run/src/__tests__/fixtures/force-tags-api/src/routes/+layout.marko (2)
1-1: LGTM! Directive correctly forces tags API.The comment directive properly instructs the Marko compiler to use the tags API for this layout template, which aligns with the test fixture's purpose.
9-12: No changes needed. The template correctly usestypeof component === "object"to distinguish between class-based and tags-based Marko APIs. In the tags API (as indicated by the fixture name and the expected snapshot output), thecomponentvariable is undefined, not null, sotypeof undefined !== "object"correctly outputs "tags". In class-based routes,componentis an object instance, so the logic outputs "class". The transform.ts file uses the identical pattern without null checks, and the snapshot verification confirms the current behavior is correct.packages/run/src/vite/__tests__/fixtures/optional-types/routes.txt (1)
3-3: Fixture update to+handler.tswith explicitverbslooks consistent.
Matches the PR’s standardized handler naming + verb metadata approach.packages/run/src/vite/__tests__/fixtures/flat-routes/routes.txt (1)
2-2: Route fixture update (generic handler + explicit verbs) is consistent..github/workflows/ci.yml (1)
18-22: CI upgrades look good; verify self-hosted runner compatibility + release auth path.
- If any self-hosted runners exist, confirm they meet
actions/checkout@v6requirements.- For the
releasejob: please confirmchangesets/action@v1can publish with your current auth (OIDC “trusted publisher” vsNODE_AUTH_TOKEN/npm token), since no npm token env var is set here.Also applies to: 37-45, 60-63, 66-70
packages/explorer/package.json (1)
18-20: Exports map reorder is harmless (same values, clearer priority).packages/run/src/vite/__tests__/fixtures/build-routes/routes.txt (1)
4-4: Fixture updates to+handler.ts+ explicit verbs are consistent across routes.Also applies to: 10-10, 12-12, 21-21, 23-23, 26-26
packages/run/src/vite/plugin.ts (1)
298-302: Marko API threading + cache invalidation look right once resolver is fixed.Please double-check
context.environment.transformRequest/context.getModuleInfoavailability across the Vite versions you support (dev vs build), since this is a Vite/Rollup API surface detail.Also applies to: 317-321, 597-597, 693-693
packages/run/src/vite/codegen/index.ts (3)
37-63:renderRouteTemplatenow emits an HTML comment in output; ensure it’s intentional beyond tests.
IfmarkoApiis only meant as test/debug metadata, consider keeping it behind a clearly test-only flag (or ensure consumers won’t treat template output as stable / parse it).
277-285:renderRouteroptionalruntimeIncludelooks fine; verify all call sites updated.
Signature change is small but breaks TS consumers if they passundefinedpositionally in older wrappers.
722-796: No regression found. Route type paths are properly decoded during path normalization. ThenormalizeSegmentfunction explicitly callsdecodeURIComponenton route segments before re-encoding reserved characters (/?#). The uri-encoded test fixture confirms this behavior is working as expected, with the generated type definitions showing correctly decoded paths like/a%2fb%3fc%23d+e:f&g/$$id`` that match the runtime router's path format. The current implementation matches the snapshot tests.packages/run/src/vite/__tests__/route-builder.test.ts (1)
10-16: Test walker wiring update is consistent with new{ fileTree, meta }shape.packages/run/src/vite/routes/walk.ts (1)
70-109:createTestWalkerroot removal is OK; watch for path prefix expectations.
Starting atdir[0]means the test tree’s root name must already be the full desired path (absolute or project-relative). Ensure no callers pass a synthetic name (e.g.,"src/routes") when they need absolute paths.packages/run/src/vite/__tests__/main.test.ts (2)
41-63:getFileDatais a nice cleanup; ensure meta key normalization matches producer.
Right now this depends onpackages/run/src/vite/__tests__/utils/fakeFS.tsproducing keys compatible withnormalizePath(routableFile.filePath).
135-142: Special routes: consider passing markoApi consistently (or document why not).
If the goal is “per-route Marko API,” special routes likely need the same behavior (or an explicit “no api for special routes” invariant).
packages/run/src/__tests__/fixtures/force-class-api/src/routes/+page.marko
Show resolved
Hide resolved
rturnq
force-pushed
the
route-entry-marko-api
branch
from
726a9f0 to
c5f786b
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
packages/run/src/__tests__/fixtures/netlify-adapter-not-edge/test.config.ts (1)
9-9: Style is correct; broader concern about unconditional skip already raised.The semicolon is properly placed. The concern about hard-coding
skip_preview = truepermanently disabling preview tests (rather than using a conditional like the commented-outprocess.env.CI === "true") was already raised in a previous review comment.packages/run/src/__tests__/fixtures/netlify-adapter-edge/test.config.ts (1)
9-9: Missing semicolon creates style inconsistency with sibling fixture.The
netlify-adapter-not-edge/test.config.tsfixture has a semicolon before the comment (true;), but this file is missing it. A past review flagged this and was marked as addressed, but the inconsistency persists.-export const skip_preview = true //process.env.CI === "true"; +export const skip_preview = true; //process.env.CI === "true";package.json (1)
52-52: Duplicate: marko@5.38.2 version issue.This version was already flagged in a previous review as non-existent on npm. See the existing review comment for details.
packages/run/src/vite/plugin.ts (1)
243-261: Address the cache logic and array access issues flagged in previous review.As noted in the previous review, this function has two critical issues:
- The cache check
if (!markoAPI)on line 247 treatsundefinedand empty string as cache misses, causing repeated recomputation and defeating the cache.- Direct access to
route.layouts[0].filePathon line 249 will throw if thelayoutsarray is empty.Please apply the fix provided in the previous review comment to ensure proper cache behavior and safe array access.
🧹 Nitpick comments (2)
examples/static/package.json (1)
22-22: Review Vite 7 migration guide for compatibility with @marko/run framework.Upgrading from Vite 6 to 7 includes breaking changes (ESM-only distribution, default build.target changes, removed plugins). While this example application doesn't directly use affected APIs (splitVendorChunkPlugin, legacy Sass), ensure @marko/run and its static adapter are compatible with Vite 7.
packages/run/src/vite/plugin.ts (1)
693-693: LGTM - Safe refactor.The direct return with non-null assertion is safe here because line 692 checks
virtualFiles.has(id)first, guaranteeing thatget(id)returns a value.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (39)
package-lock.jsonis excluded by!**/package-lock.jsonand included by**packages/run/src/__tests__/fixtures/force-class-api/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/force-class-api/__snapshots__/preview.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/force-tags-api/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/force-tags-api/__snapshots__/preview.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/basic-500/__snapshots__/basic-500.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/basic-500/__snapshots__/basic-500.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/basic/__snapshots__/basic.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/basic/__snapshots__/basic.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/build-routes/__snapshots__/build-routes.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/build-routes/__snapshots__/build-routes.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/build-routes/__snapshots__/build-routes.expected.routetypes.d.tsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/escape-path/__snapshots__/escape-path.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/escape-path/__snapshots__/escape-path.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/flat-routes/__snapshots__/flat-routes.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/flat-routes/__snapshots__/flat-routes.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/flat-routes/__snapshots__/flat-routes.expected.routetypes.d.tsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-class-api/__snapshots__/force-class-api.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-class-api/__snapshots__/force-class-api.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-class-api/__snapshots__/force-class-api.expected.routetypes.d.tsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-tags-api/__snapshots__/force-tags-api.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-tags-api/__snapshots__/force-tags-api.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-tags-api/__snapshots__/force-tags-api.expected.routetypes.d.tsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/multiple-sources/__snapshots__/multiple-sources.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/multiple-sources/__snapshots__/multiple-sources.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/nested-dynamic/__snapshots__/nested-dynamic.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/nested-dynamic/__snapshots__/nested-dynamic.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/nested-dynamic/__snapshots__/nested-dynamic.expected.routetypes.d.tsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/optional-dynamic/__snapshots__/optional-dynamic.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/optional-dynamic/__snapshots__/optional-dynamic.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/optional-static/__snapshots__/optional-static.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/optional-static/__snapshots__/optional-static.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/optional-types/__snapshots__/optional-types.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/optional-types/__snapshots__/optional-types.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/optional-types/__snapshots__/optional-types.expected.routetypes.d.tsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/param-optional-rest/__snapshots__/param-optional-rest.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/param-optional-rest/__snapshots__/param-optional-rest.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/uri-encoded/__snapshots__/uri-encoded.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/uri-encoded/__snapshots__/uri-encoded.expected.routes.mdis excluded by!**/__snapshots__/**and included by**
📒 Files selected for processing (40)
.changeset/spicy-moles-stop.md(1 hunks).github/workflows/ci.yml(3 hunks)examples/netlify/package.json(1 hunks)examples/node-express/package.json(1 hunks)examples/static/package.json(1 hunks)examples/zero-config/package.json(1 hunks)package.json(1 hunks)packages/explorer/package.json(1 hunks)packages/run/package.json(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/.marko-run/routes.d.ts(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/marko.json(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/src/routes/+layout.marko(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/src/routes/+page.marko(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/test.config.ts(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/transform.ts(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/tsconfig.json(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/.marko-run/routes.d.ts(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/marko.json(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/src/routes/+layout.marko(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/src/routes/+page.marko(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/test.config.ts(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/transform.ts(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/tsconfig.json(1 hunks)packages/run/src/__tests__/fixtures/netlify-adapter-edge/netlify.toml(1 hunks)packages/run/src/__tests__/fixtures/netlify-adapter-edge/test.config.ts(1 hunks)packages/run/src/__tests__/fixtures/netlify-adapter-not-edge/test.config.ts(1 hunks)packages/run/src/__tests__/main.test.ts(1 hunks)packages/run/src/vite/__tests__/fixtures/build-routes/routes.txt(2 hunks)packages/run/src/vite/__tests__/fixtures/flat-routes/routes.txt(1 hunks)packages/run/src/vite/__tests__/fixtures/force-class-api/routes.txt(1 hunks)packages/run/src/vite/__tests__/fixtures/force-tags-api/routes.txt(1 hunks)packages/run/src/vite/__tests__/fixtures/nested-dynamic/routes.txt(1 hunks)packages/run/src/vite/__tests__/fixtures/optional-types/routes.txt(1 hunks)packages/run/src/vite/__tests__/main.test.ts(5 hunks)packages/run/src/vite/__tests__/route-builder.test.ts(1 hunks)packages/run/src/vite/__tests__/utils/fakeFS.ts(2 hunks)packages/run/src/vite/codegen/index.ts(3 hunks)packages/run/src/vite/plugin.ts(5 hunks)packages/run/src/vite/routes/walk.ts(2 hunks)packages/run/src/vite/utils/log.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/run/src/tests/fixtures/force-tags-api/marko.json
🚧 Files skipped from review as they are similar to previous changes (23)
- packages/run/src/tests/fixtures/force-tags-api/src/routes/+page.marko
- packages/run/src/vite/tests/utils/fakeFS.ts
- packages/run/src/tests/fixtures/force-class-api/transform.ts
- .changeset/spicy-moles-stop.md
- packages/run/src/vite/utils/log.ts
- packages/run/src/tests/fixtures/force-class-api/test.config.ts
- packages/run/src/tests/fixtures/netlify-adapter-edge/netlify.toml
- packages/run/src/vite/tests/route-builder.test.ts
- packages/run/src/vite/codegen/index.ts
- examples/node-express/package.json
- examples/netlify/package.json
- packages/explorer/package.json
- packages/run/src/tests/fixtures/force-class-api/src/routes/+layout.marko
- packages/run/src/vite/tests/fixtures/force-tags-api/routes.txt
- packages/run/src/tests/fixtures/force-class-api/src/routes/+page.marko
- packages/run/src/tests/fixtures/force-tags-api/.marko-run/routes.d.ts
- examples/zero-config/package.json
- packages/run/src/vite/tests/fixtures/force-class-api/routes.txt
- packages/run/src/tests/fixtures/force-tags-api/tsconfig.json
- packages/run/src/tests/fixtures/force-class-api/tsconfig.json
- packages/run/src/tests/fixtures/force-tags-api/test.config.ts
- packages/run/src/tests/main.test.ts
- packages/run/src/tests/fixtures/force-tags-api/transform.ts
🧰 Additional context used
🧬 Code graph analysis (4)
packages/run/src/vite/__tests__/main.test.ts (2)
packages/run/src/vite/types.ts (2)
RoutableFile(112-118)HttpVerb(11-11)packages/run/src/vite/constants.ts (2)
HttpVerb(29-29)httpVerbs(8-16)
packages/run/src/__tests__/fixtures/netlify-adapter-not-edge/test.config.ts (1)
packages/run/src/__tests__/fixtures/netlify-adapter-edge/test.config.ts (1)
skip_preview(9-9)
packages/run/src/vite/plugin.ts (2)
packages/run/src/vite/types.ts (1)
Route(87-97)packages/run/src/vite/codegen/index.ts (1)
renderRouteTemplate(37-63)
packages/run/src/__tests__/fixtures/netlify-adapter-edge/test.config.ts (1)
packages/run/src/__tests__/fixtures/netlify-adapter-not-edge/test.config.ts (1)
skip_preview(9-9)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test: node@20 (windows-latest)
- GitHub Check: test: node@24 (ubuntu-latest)
- GitHub Check: test: node@20 (ubuntu-latest)
- GitHub Check: test: node@24 (windows-latest)
- GitHub Check: test: node@22 (windows-latest)
- GitHub Check: test: node@22 (ubuntu-latest)
🔇 Additional comments (24)
packages/run/src/__tests__/fixtures/force-class-api/marko.json (1)
1-3: Fixture config looks correct; please verify Marko config key/path for the repo’s Marko version.Since this relies on Marko’s config semantics, please double-check that
marko.jsonsupports the"transform"key here and that./transform.tsresolves as expected in the test harness.packages/run/src/vite/__tests__/fixtures/build-routes/routes.txt (5)
4-4: Fixture change looks consistent, but ensure the verbs-metadata parser accepts inline JSON exactly like this.This moves
/newto+handler.ts { "verbs": ["post"] }; please confirm the test-route parser forroutes.txtsupports inline JSON with this spacing/quoting (since this is fixture-driven).
10-10: Double-check intent: same verbs on nested/$idand/commentshandlers.Both
/notes/$id(Line 12) and/notes/$id/comments(Line 10) declare["put","post","delete"]. If this is meant to model two distinct handlers with identical verb coverage, OK; otherwise it may be a fixture duplication that could mask routing precedence bugs.Also applies to: 12-12
21-21: LGTM: single-verb route expressed via metadata.
/callback/oauth2now declares["get"], which matches the new “generic handler + verbs” pattern cleanly.
23-23: Verify GET+HEAD semantics remain correct with metadata.
/myuses["get","head"]; please confirm the router still treatsheadcorrectly (and doesn’t implicitly add/override it fromget, depending on prior behavior).
26-26: LGTM:$$matchhandler verbs metadata is straightforward.Looks consistent with the rest of the fixture updates.
packages/run/src/__tests__/fixtures/force-class-api/.marko-run/routes.d.ts (1)
1-39: Generated type definitions look correct for the test fixture.This auto-generated type definitions file properly:
- Augments
@marko/runwith the route structure for the "/" path- Provides type-safe
MarkoRunnamespace exports for both+page.markoand+layout.marko- Includes the
Inputinterface for the layout component using the standardRun.LayoutInputpattern- Marks the older
routeexport pattern as deprecated with clear migration guidanceThe structure is consistent and follows TypeScript module augmentation conventions correctly.
packages/run/src/__tests__/fixtures/force-tags-api/src/routes/+layout.marko (4)
1-1: LGTM!The
<!-- use tags -->directive correctly forces the tags API mode, which aligns with the fixture's purpose.
2-9: LGTM!The HTML structure and Marko shorthand syntax (
<div#app>) are correct and appropriate for a layout template.
11-14: LGTM!The dynamic content rendering using
<${input.content}/>is standard Marko syntax and appropriate for a layout template that wraps page content.
10-10: No action needed. Thetypeofoperator safely evaluates undefined variables without throwing aReferenceError. The code correctly handles both cases: whencomponentis undefined,typeof component === "object"returns false and outputs"tags"; whencomponentis an object, it outputs"class". This pattern is intentional and works as designed.packages/run/src/vite/__tests__/fixtures/nested-dynamic/routes.txt (2)
4-4: LGTM: Corrected dynamic segment naming.The change from
/$bar Id(with space) to/$bar_Id(with underscore) fixes the dynamic route parameter syntax.
8-8: LGTM: Test coverage enhancement.Adding
+page.markounder the nested dynamic route path extends test coverage.packages/run/src/vite/__tests__/fixtures/flat-routes/routes.txt (1)
2-2: LGTM: Clearer verb specification via metadata.Moving from filename-based verb encoding (
+handler.get_post.marko) to explicit JSON metadata ({ "verbs": ["get", "post"] }) improves clarity and maintainability..github/workflows/ci.yml (3)
37-37: LGTM: Extended Node version test coverage.Adding Node 24 to the test matrix ensures compatibility with the latest LTS release.
60-63: LGTM: Appropriate permissions for release workflow.The added permissions (id-token, contents, pull-requests) are correctly scoped for a release job that publishes packages and manages changesets.
18-23: All versions verified as available and stable.actions/checkout@v6 and actions/setup-node@v6 are released and marked stable in the GitHub Marketplace. Node.js 24 was released in May 2025 and is currently the Active LTS line recommended for production use.
packages/run/src/vite/__tests__/main.test.ts (4)
2-6: LGTM: Import updates for enhanced type safety.The import changes (mocha-snap default import to named
snapexport and addingRoutableFiletype) support the test infrastructure improvements.
39-45: LGTM: Well-structured metadata infrastructure.The
jsonDataMap and type-safegetFileDatahelper provide a clean approach for managing per-file route metadata in tests.
57-61: LGTM: Proper integration with updated walker API.The updated
createTestWalkercall correctly uses the new signature and provides a callback to populate thejsonDataMap during directory traversal.
108-114: LGTM: Robust verb extraction with validation.The metadata-driven verb extraction includes proper filtering against valid
httpVerbsand appropriate fallback logic.examples/static/package.json (1)
21-21: Remove this comment - version change is an upgrade, not a downgrade.The version change from
^5.7.2to^5.9.3is an upgrade (5.9.3 > 5.7.2), not a downgrade. The review comment's core claim is incorrect.packages/run/src/vite/routes/walk.ts (1)
70-109: AllcreateTestWalkercall sites have been properly updated.The function signature change from
(dir: TestFileTree, root: string)to(dir: TestFileTree)is correctly reflected across all call sites. Found 2 usages inmain.test.tsandroute-builder.test.ts, both passing only the singledirargument as expected.packages/run/package.json (1)
36-36: Bump @marko/vite to ^5.4.2 — patch release, no breaking changes.
5.4.2 is a patch release with fixes (renderBuiltUrl relative URL handling, originalFilename support) and no listed breaking changes; safe to upgrade.
rturnq
force-pushed
the
route-entry-marko-api
branch
from
c5f786b to
f22893d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
examples/zero-config/package.json (1)
12-12: marko@5.38.2 does not exist on npm—installation will fail.This version is not published on the npm registry. The latest 5.x releases are in the 5.37.x series. Consult the root package.json change or the PR description to understand the intended version, then update to a valid published release.
package.json (1)
52-52: marko@5.38.2 does not exist on npm—installation will fail.This version is not published on the npm registry. The latest 5.x releases are in the 5.37.x series (e.g., 5.37.63), and marko 6.0.39 is the latest stable release. Resolve this to an actual published version before merge.
packages/run/src/__tests__/fixtures/netlify-adapter-edge/netlify.toml (1)
1-4: Makebaserelative (fixture portability / cwd correctness).This fixture-level
base = "packages/run/src/__tests__/fixtures/netlify-adapter-edge"is brittle if the Netlify CLI runs withcwdset to the fixture directory (or from another checkout location).[build] -base = "packages/run/src/__tests__/fixtures/netlify-adapter-edge" publish = "dist/public" edge_functions = "dist"(Alternative:
base = ".".)packages/run/src/__tests__/fixtures/netlify-adapter-edge/test.config.ts (1)
8-9: Don’t permanently disable preview tests; also fix semicolon.Hard-coding
skip_preview = trueturns off preview coverage everywhere; if the intent is “skip in CI only” (as the comment suggests), restore that (or use an explicit env flag). Also add the missing semicolon to match the other fixture.export const preview_args = ["--offline", "--no-open"]; -export const skip_preview = true //process.env.CI === "true"; +export const skip_preview = process.env.CI === "true";If you prefer explicit local opt-in instead:
-export const skip_preview = true //process.env.CI === "true"; +export const skip_preview = process.env.RUN_E2E_PREVIEW !== "true";Please confirm which behavior you want as the default.
packages/run/src/__tests__/fixtures/netlify-adapter-not-edge/test.config.ts (1)
8-9: Avoid hard-codedskip_preview = true(masks preview regressions).This disables preview tests entirely; if the intent is “skip only on CI”, restore the env-based condition (and keep both fixtures consistent):
export const preview_args = ["--offline", "--no-open"]; -export const skip_preview = true; //process.env.CI === "true"; +export const skip_preview = process.env.CI === "true";Please confirm the desired default (run locally vs only run when explicitly enabled).
🧹 Nitpick comments (1)
packages/run/src/vite/__tests__/utils/fakeFS.ts (1)
1-1: Usepath.join()for more robust path construction.While importing
sepaddresses the cross-platform separator concern from the previous review, the manual concatenation pattern on line 51 still has edge cases. WhenbaseDiris empty (the default),dirStack.join(sep)returns"", and"" + sep + nameproduces a leading separator (/nameon Unix,\nameon Windows), which may not be the intended behavior for a relative path.Apply this diff to use the standard
path.join()function:-import { sep } from "path"; +import path from "path";} else { - if (onData && data) { - onData(`${dirStack.join(sep)}${sep}${name}`, data); - } + if (onData && data) { + onData(path.join(...dirStack, name), data); + } current[1].push(name); }This approach correctly handles empty base directories, normalizes separators, and is more idiomatic.
Also applies to: 51-51
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.jsonand included by**
📒 Files selected for processing (12)
examples/netlify/package.json(1 hunks)examples/node-express/package.json(1 hunks)examples/static/package.json(1 hunks)examples/zero-config/package.json(1 hunks)package.json(1 hunks)packages/run/src/__tests__/fixtures/netlify-adapter-edge/netlify.toml(1 hunks)packages/run/src/__tests__/fixtures/netlify-adapter-edge/test.config.ts(1 hunks)packages/run/src/__tests__/fixtures/netlify-adapter-not-edge/test.config.ts(1 hunks)packages/run/src/__tests__/main.test.ts(1 hunks)packages/run/src/vite/__tests__/main.test.ts(5 hunks)packages/run/src/vite/__tests__/utils/fakeFS.ts(2 hunks)packages/run/src/vite/utils/log.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/run/src/vite/utils/log.ts
- examples/node-express/package.json
- examples/netlify/package.json
- packages/run/src/tests/main.test.ts
- examples/static/package.json
- packages/run/src/vite/tests/main.test.ts
🧰 Additional context used
🧬 Code graph analysis (3)
packages/run/src/__tests__/fixtures/netlify-adapter-edge/test.config.ts (1)
packages/run/src/__tests__/fixtures/netlify-adapter-not-edge/test.config.ts (1)
skip_preview(9-9)
packages/run/src/__tests__/fixtures/netlify-adapter-not-edge/test.config.ts (1)
packages/run/src/__tests__/fixtures/netlify-adapter-edge/test.config.ts (1)
skip_preview(9-9)
packages/run/src/vite/__tests__/utils/fakeFS.ts (1)
packages/run/src/vite/routes/walk.ts (1)
TestFileTree(68-68)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test: node@20 (windows-latest)
- GitHub Check: test: node@22 (windows-latest)
- GitHub Check: test: node@24 (ubuntu-latest)
- GitHub Check: test: node@24 (windows-latest)
- GitHub Check: test: node@22 (ubuntu-latest)
- GitHub Check: test: node@20 (ubuntu-latest)
🔇 Additional comments (1)
packages/run/src/vite/__tests__/utils/fakeFS.ts (1)
5-9: No action needed. TheonDataparameter is optional and the function properly handles both cases—call sites that provide the callback (main.test.ts) and those that don't (route-builder.test.ts) both work correctly.
rturnq
force-pushed
the
route-entry-marko-api
branch
from
f22893d to
c5edced
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
packages/run/src/__tests__/fixtures/netlify-adapter-edge/netlify.toml (1)
2-2: Absolute path inbasefield should use relative path (duplicate concern).This is the same issue flagged in the previous review: the
basefield uses an absolute repo path (packages/run/src/__tests__/fixtures/netlify-adapter-edge) which may not resolve correctly when the Netlify CLI is invoked from the fixture directory. Even though preview testing is currently disabled (per the enriched summary), this should be corrected for future compatibility by using a relative path likebase = "."or by omitting thebaseline entirely so paths resolve relative to the fixture's working directory.Apply this diff to use a relative path:
[build] -base = "packages/run/src/__tests__/fixtures/netlify-adapter-edge" +base = "." publish = "dist/public" edge_functions = "dist"Alternatively, omit
baseentirely and rely on the fixture directory as the root.packages/run/src/vite/__tests__/utils/fakeFS.ts (1)
50-52: Path construction still has edge-case issues; complete the fix suggested in previous review.While the code now uses
sepinstead of hard-coded/, the string interpolation approach produces incorrect paths whenbaseDiris empty:
- When
baseDir = "",dirStackstarts as[""]- Joining with separators produces a leading separator:
["", "dir"].join(sep) + sep + name→/dir/name(Unix) or\dir\name(Windows)The previous review suggested using
path.join(...dirStack, name), which correctly handles empty path components.Apply this diff to complete the fix:
- if (onData && data) { - onData(`${dirStack.join(sep)}${sep}${name}`, data); - } + if (onData && data) { + onData(join(...dirStack, name), data); + }Note: This assumes you import
joinfrom the path module (see comment on line 1).packages/run/src/vite/utils/log.ts (1)
185-187: Avoid allocating aBlobjust to compute byte size (Line 186).
This adds avoidable allocation/GC; preferBuffer.byteLength(string) /Uint8Array#byteLength.function byteSize(source: string | Uint8Array): number { - return new Blob([source as BlobPart]).size; + return typeof source === "string" ? Buffer.byteLength(source) : source.byteLength; }packages/run/src/vite/plugin.ts (2)
580-615: Layout edits don’t invalidaterouteMarkoApiCache(stale Marko API until restart) (Line 592-599).
You probably wantRoutableFileTypes.Layoutchanges to trigger the same rebuild/invalidation path.type === "add" || type === "unlink" || (type === "change" && - (routableFileType === RoutableFileTypes.Handler || + (routableFileType === RoutableFileTypes.Handler || routableFileType === RoutableFileTypes.Middleware || + routableFileType === RoutableFileTypes.Layout || filename === runtimeInclude))
144-156: Fix possible crash when a route has no layouts (Line 150).
route.layouts[0].filePathwill throw iflayoutsis empty.async function getMarkoApiForRoute(context: PluginContext, route: Route) { routeMarkoApiCache ??= new Map(); if (!routeMarkoApiCache.has(route)) { + const topLayout = route.layouts[0]; const markoAPI = route.templateFilePath && - ((await loadModule(context, route.layouts[0].filePath))?.meta + topLayout && + ((await loadModule(context, topLayout.filePath))?.meta ?.markoAPI as string | undefined); routeMarkoApiCache.set(route, markoAPI); return markoAPI; } return routeMarkoApiCache.get(route); }package.json (1)
25-65:marko@^5.38.2still looks unpublished; this will breaknpm i(Line 52). (npmjs.com)#!/bin/bash set -euo pipefail npm view marko version npm view marko@5.38.2 versionpackages/run/src/vite/__tests__/main.test.ts (1)
120-126: Guardroute.layouts[0]before reading it (Line 122).
This can throw for routes with no layouts.- const layoutData = getFileData<{ api: string }>(route.layouts[0]); + const layoutData = getFileData<{ api: string }>(route.layouts[0]); routesSnap += "### Template\n"; routesSnap += "```marko\n"; routesSnap += renderRouteTemplate(route, layoutData?.api);(With the
getFileData(file: RoutableFile | undefined)change above,route.layouts[0]is safe.)
🧹 Nitpick comments (6)
packages/run/src/vite/__tests__/utils/fakeFS.ts (1)
1-1: Consider importingjoinfrom path module for robust path construction.While importing
sepaddresses part of the cross-platform concern from the previous review, importingjoinwould enable more idiomatic and safer path construction (see comment on lines 50-52).Apply this diff:
-import { sep } from "path"; +import { join, sep } from "path";packages/run/src/__tests__/fixtures/force-class-api/transform.ts (1)
1-40: Guardprogram.hub.file.opts.filenameto avoid crashing transforms in non-file contexts.If this transform ever runs with an undefined filename (e.g., synthetic AST in tooling),
RegExp.test(undefined)will throw. A tiny guard keeps the fixture robust without changing intent.export default { Program(program: t.NodePath<t.Program>) { + const filename = program.hub.file?.opts?.filename; if ( /dist\/\.marko-run\/.+(?<!entry)\.marko$/.test( - program.hub.file.opts.filename, + filename ?? "", ) ) { program.node.body.push( t.markoPlaceholder( t.templateLiteral( [.github/workflows/ci.yml (1)
35-38: Node “20” in matrix: ensure it satisfies Vite 7’s Node >=20.19 requirement (Line 37). (vite.dev)
Consider pinning to20.19(or20.19.x) if you want to make the constraint explicit.package.json (1)
25-65: Withvite@^7, consider aligningengines.node(if you have one) to Vite 7 minimums. (vite.dev)
Not strictly required, but it prevents “works on my machine” installs with too-old Node 20.packages/run/src/vite/__tests__/main.test.ts (2)
39-45: MakegetFileDatareturnT | undefined(don’tas Ta missing map entry) (Line 41-45).- function getFileData<T extends Record<string, unknown>>( - file: RoutableFile, - ) { - return jsonData.get(file.filePath) as T; - } + function getFileData<T extends Record<string, unknown>>( + file: RoutableFile | undefined, + ): T | undefined { + return file ? (jsonData.get(file.filePath) as T | undefined) : undefined; + }
136-143: Special-route template snapshots may need Marko API too (parity with normal routes) (Line 141).
If special routes can be affected by top-layout API, consider passinglayoutData?.apihere as well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (11)
package-lock.jsonis excluded by!**/package-lock.jsonand included by**packages/run/src/__tests__/fixtures/force-class-api/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/force-class-api/__snapshots__/preview.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/force-tags-api/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/force-tags-api/__snapshots__/preview.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-class-api/__snapshots__/force-class-api.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-class-api/__snapshots__/force-class-api.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-class-api/__snapshots__/force-class-api.expected.routetypes.d.tsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-tags-api/__snapshots__/force-tags-api.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-tags-api/__snapshots__/force-tags-api.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-tags-api/__snapshots__/force-tags-api.expected.routetypes.d.tsis excluded by!**/__snapshots__/**and included by**
📒 Files selected for processing (33)
.changeset/spicy-moles-stop.md(1 hunks).github/workflows/ci.yml(3 hunks)examples/netlify/package.json(1 hunks)examples/node-express/package.json(1 hunks)examples/static/package.json(1 hunks)examples/zero-config/package.json(1 hunks)package.json(1 hunks)packages/run/package.json(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/.marko-run/routes.d.ts(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/marko.json(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/src/routes/+layout.marko(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/src/routes/+page.marko(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/test.config.ts(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/transform.ts(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/tsconfig.json(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/.marko-run/routes.d.ts(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/marko.json(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/src/routes/+layout.marko(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/src/routes/+page.marko(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/test.config.ts(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/transform.ts(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/tsconfig.json(1 hunks)packages/run/src/__tests__/fixtures/netlify-adapter-edge/netlify.toml(1 hunks)packages/run/src/__tests__/fixtures/netlify-adapter-edge/test.config.ts(1 hunks)packages/run/src/__tests__/fixtures/netlify-adapter-not-edge/test.config.ts(1 hunks)packages/run/src/__tests__/main.test.ts(1 hunks)packages/run/src/vite/__tests__/fixtures/force-class-api/routes.txt(1 hunks)packages/run/src/vite/__tests__/fixtures/force-tags-api/routes.txt(1 hunks)packages/run/src/vite/__tests__/main.test.ts(5 hunks)packages/run/src/vite/__tests__/utils/fakeFS.ts(2 hunks)packages/run/src/vite/codegen/index.ts(3 hunks)packages/run/src/vite/plugin.ts(6 hunks)packages/run/src/vite/utils/log.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (17)
- packages/run/package.json
- examples/zero-config/package.json
- packages/run/src/tests/fixtures/force-tags-api/transform.ts
- packages/run/src/tests/fixtures/force-tags-api/src/routes/+page.marko
- packages/run/src/tests/fixtures/force-class-api/tsconfig.json
- packages/run/src/tests/fixtures/force-class-api/test.config.ts
- packages/run/src/vite/codegen/index.ts
- packages/run/src/tests/fixtures/force-class-api/src/routes/+page.marko
- packages/run/src/vite/tests/fixtures/force-class-api/routes.txt
- packages/run/src/tests/fixtures/force-tags-api/src/routes/+layout.marko
- examples/static/package.json
- packages/run/src/tests/main.test.ts
- packages/run/src/tests/fixtures/force-class-api/src/routes/+layout.marko
- packages/run/src/tests/fixtures/netlify-adapter-not-edge/test.config.ts
- .changeset/spicy-moles-stop.md
- examples/node-express/package.json
- packages/run/src/tests/fixtures/netlify-adapter-edge/test.config.ts
🧰 Additional context used
🧬 Code graph analysis (6)
packages/run/src/vite/__tests__/main.test.ts (4)
packages/run/src/vite/types.ts (2)
RoutableFile(112-118)HttpVerb(11-11)packages/run/src/vite/routes/walk.ts (1)
createTestWalker(70-110)packages/run/src/vite/constants.ts (2)
HttpVerb(29-29)httpVerbs(8-16)packages/run/src/vite/codegen/index.ts (1)
renderRouteTemplate(37-63)
packages/run/src/vite/plugin.ts (2)
packages/run/src/vite/types.ts (1)
Route(87-97)packages/run/src/vite/codegen/index.ts (1)
renderRouteTemplate(37-63)
packages/run/src/vite/__tests__/utils/fakeFS.ts (1)
packages/run/src/vite/routes/walk.ts (1)
TestFileTree(68-68)
packages/run/src/__tests__/fixtures/force-class-api/transform.ts (1)
packages/run/src/__tests__/fixtures/force-tags-api/transform.ts (1)
Program(4-39)
packages/run/src/__tests__/fixtures/force-tags-api/test.config.ts (1)
packages/run/src/__tests__/fixtures/force-class-api/test.config.ts (1)
steps(1-1)
packages/run/src/__tests__/fixtures/force-class-api/.marko-run/routes.d.ts (2)
packages/run/src/vite/__tests__/fixtures/force-class-api/__snapshots__/force-class-api.expected.routetypes.d.ts (9)
Route(21-21)Route(33-33)Context(22-22)Context(34-34)Handler(23-23)Handler(35-35)route(25-25)route(37-37)Input(30-30)packages/run/src/vite/__tests__/fixtures/force-tags-api/__snapshots__/force-tags-api.expected.routetypes.d.ts (9)
Route(21-21)Route(33-33)Context(22-22)Context(34-34)Handler(23-23)Handler(35-35)route(25-25)route(37-37)Input(30-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test: node@24 (ubuntu-latest)
- GitHub Check: test: node@24 (windows-latest)
- GitHub Check: test: node@20 (ubuntu-latest)
- GitHub Check: test: node@20 (windows-latest)
- GitHub Check: test: node@22 (windows-latest)
- GitHub Check: test: node@22 (ubuntu-latest)
🔇 Additional comments (9)
packages/run/src/__tests__/fixtures/force-tags-api/test.config.ts (1)
1-1: Good fixture parity with force-class-api.packages/run/src/__tests__/fixtures/force-class-api/.marko-run/routes.d.ts (1)
1-39: LGTM (generated fixture matches the expected route typing shape).packages/run/src/__tests__/fixtures/force-tags-api/.marko-run/routes.d.ts (1)
1-39: Generated typings are correct;@marko/run/namespaceexports all required symbols.The fixture imports are verified as correct.
NotHandled,NotMatched,GetPaths,PostPaths,GetablePath,GetableHref,PostablePath,PostableHref, andPlatformare all properly exported from@marko/run/namespaceand the module augmentation declarations match the expected route structure.examples/netlify/package.json (2)
14-24: Netlify example + Vite 7: ensure the build Node version meets Vite 7 minimum (Line 23). (vite.dev)
If your Netlify site/build image is still on Node 20 but <20.19, this will be flaky.
11-24:marko@^5.38.2is published and available on npm—no installation issues with this dependency..github/workflows/ci.yml (2)
18-23: CI action upgrades look good; note self-hosted runner minimums (Line 18, 20, 40, 42, 66, 68). (github.com)
If you support self-hosted runners, confirm they’re on a new enough Actions Runner for Node24-based actions.Also applies to: 40-45, 66-71
60-64: Good hardening: explicitpermissions+ no NPM token exposure (Line 60-63).Also applies to: 82-83
packages/run/src/vite/__tests__/main.test.ts (1)
107-115: Handler verb override via metadata looks good (Line 108-115).packages/run/src/vite/plugin.ts (1)
124-130:loadModuleis fine, but confirm typings forcontext.environmentare sound (Line 125-129).
Rollup’sPluginContextdoesn’t generally exposeenvironment; ensure your Vite types/augments cover this. (github.com)
rturnq
force-pushed
the
route-entry-marko-api
branch
from
c5edced to
9431c17
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (9)
packages/run/src/__tests__/fixtures/netlify-adapter-not-edge/test.config.ts (1)
9-9: Preview tests are now permanently disabled for this fixture
skip_previewbeing hard-coded totruerisks masking regressions; the trailing commented-outprocess.env.CI === "true"reads like an unfinished change. Please either restore an explicit env-gated condition (or an opt-in flag), or add a clear comment explaining why preview must never run here.-export const skip_preview = true; //process.env.CI === "true"; +// Preview is expensive/flaky in CI; run it explicitly when needed. +export const skip_preview = process.env.RUN_E2E_PREVIEW !== "true";packages/run/src/__tests__/fixtures/netlify-adapter-edge/test.config.ts (1)
9-9: Same “preview permanently skipped” concern + semicolon inconsistencyThis still unconditionally skips preview (can mask regressions), and it’s inconsistent with the not-edge fixture due to the missing semicolon before the trailing comment.
-export const skip_preview = true //process.env.CI === "true"; +// Preview is expensive/flaky in CI; run it explicitly when needed. +export const skip_preview = process.env.RUN_E2E_PREVIEW !== "true"; +// (previous logic) process.env.CI === "true"If you do want it always skipped, please at least make it consistent/readable:
-export const skip_preview = true //process.env.CI === "true"; +export const skip_preview = true; // process.env.CI === "true";packages/run/src/__tests__/fixtures/force-class-api/marko.json (1)
1-3: The.tstransform reference has already been flagged in previous review.As noted in the earlier review, this fixture references
"./transform.ts"which works in the test environment due to thetsxloader, but deviates from Marko's documented pattern of using.jsfiles. The previous review recommendation to compile the transformer and reference the compiled output remains valid.packages/run/src/__tests__/fixtures/force-tags-api/transform.ts (1)
5-9: Guard against undefined filename to prevent TypeError.The existing review comment correctly identifies that
program.hub.file.opts.filenamecould be undefined, which would cause.test()to throw. This should be addressed.packages/run/src/__tests__/fixtures/force-tags-api/tsconfig.json (1)
1-4: Remove trailing comma for strict JSON compatibility.The existing review comment correctly identifies the trailing comma after the "include" array, which can cause issues with strict JSON parsers.
examples/zero-config/package.json (1)
12-12: Verifymarko@^5.38.2is published and intended for this repo.
Past reviews raised that this version didn’t exist at the time; please re-confirm before merging.Is npm package "marko" version 5.38.2 published? If yes, what is the latest 5.x version and what versions are available around 5.38.x?package.json (1)
52-55: Re-checkmarko@^5.38.2existence + keepmocha-snapusage consistent.
This was flagged previously (and may have changed since), but worth confirming again before merging.1) Is npm package "marko" version 5.38.2 published? 2) For mocha-snap v5.x, is the correct usage `import snap from "mocha-snap"` or `import { snap } from "mocha-snap"`?packages/run/src/vite/plugin.ts (2)
599-599: Cache invalidation likely misses “layout content changed” → stale markoAPI.
SincemarkoAPIis read from the layout module meta, achangeto a layout file should also resetrouteMarkoApiCache(or broaden the invalidation to anyroutableFileType).- if ( + if ( type === "add" || type === "unlink" || (type === "change" && (routableFileType === RoutableFileTypes.Handler || routableFileType === RoutableFileTypes.Middleware || + routableFileType === RoutableFileTypes.Layout || filename === runtimeInclude)) ) {
144-156: Guardroute.layouts[0]to avoid crashing on routes without layouts.
Right nowroute.layouts[0].filePathwill throw whenlayoutsis empty. Also consider what the correct fallback API should be (e.g.undefined, or infer fromroute.page).async function getMarkoApiForRoute(context: PluginContext, route: Route) { routeMarkoApiCache ??= new Map(); if (!routeMarkoApiCache.has(route)) { - const markoAPI = - route.templateFilePath && - ((await loadModule(context, route.layouts[0].filePath))?.meta - ?.markoAPI as string | undefined); + const topLayout = route.layouts[0]; + const markoAPI = + route.templateFilePath && + topLayout && + ((await loadModule(context, topLayout.filePath))?.meta?.markoAPI as + | string + | undefined); routeMarkoApiCache.set(route, markoAPI); return markoAPI; } return routeMarkoApiCache.get(route); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (11)
package-lock.jsonis excluded by!**/package-lock.jsonand included by**packages/run/src/__tests__/fixtures/force-class-api/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/force-class-api/__snapshots__/preview.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/force-tags-api/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/force-tags-api/__snapshots__/preview.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-class-api/__snapshots__/force-class-api.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-class-api/__snapshots__/force-class-api.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-class-api/__snapshots__/force-class-api.expected.routetypes.d.tsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-tags-api/__snapshots__/force-tags-api.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-tags-api/__snapshots__/force-tags-api.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-tags-api/__snapshots__/force-tags-api.expected.routetypes.d.tsis excluded by!**/__snapshots__/**and included by**
📒 Files selected for processing (33)
.changeset/spicy-moles-stop.md(1 hunks).github/workflows/ci.yml(3 hunks)examples/netlify/package.json(1 hunks)examples/node-express/package.json(1 hunks)examples/static/package.json(1 hunks)examples/zero-config/package.json(1 hunks)package.json(1 hunks)packages/run/package.json(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/.marko-run/routes.d.ts(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/marko.json(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/src/routes/+layout.marko(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/src/routes/+page.marko(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/test.config.ts(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/transform.ts(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/tsconfig.json(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/.marko-run/routes.d.ts(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/marko.json(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/src/routes/+layout.marko(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/src/routes/+page.marko(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/test.config.ts(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/transform.ts(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/tsconfig.json(1 hunks)packages/run/src/__tests__/fixtures/netlify-adapter-edge/netlify.toml(1 hunks)packages/run/src/__tests__/fixtures/netlify-adapter-edge/test.config.ts(1 hunks)packages/run/src/__tests__/fixtures/netlify-adapter-not-edge/test.config.ts(1 hunks)packages/run/src/__tests__/main.test.ts(1 hunks)packages/run/src/vite/__tests__/fixtures/force-class-api/routes.txt(1 hunks)packages/run/src/vite/__tests__/fixtures/force-tags-api/routes.txt(1 hunks)packages/run/src/vite/__tests__/main.test.ts(5 hunks)packages/run/src/vite/__tests__/utils/fakeFS.ts(2 hunks)packages/run/src/vite/codegen/index.ts(3 hunks)packages/run/src/vite/plugin.ts(6 hunks)packages/run/src/vite/utils/log.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- examples/static/package.json
- packages/run/src/tests/fixtures/force-class-api/src/routes/+page.marko
- examples/netlify/package.json
- packages/run/src/tests/fixtures/netlify-adapter-edge/netlify.toml
- packages/run/src/vite/utils/log.ts
- packages/run/src/tests/fixtures/force-tags-api/test.config.ts
- packages/run/src/tests/fixtures/force-class-api/transform.ts
- packages/run/src/tests/fixtures/force-tags-api/src/routes/+layout.marko
- .github/workflows/ci.yml
- packages/run/src/tests/fixtures/force-tags-api/src/routes/+page.marko
- packages/run/src/tests/fixtures/force-class-api/tsconfig.json
- packages/run/src/tests/fixtures/force-class-api/test.config.ts
- packages/run/src/vite/tests/utils/fakeFS.ts
- packages/run/src/vite/tests/main.test.ts
- packages/run/src/tests/fixtures/force-class-api/src/routes/+layout.marko
🧰 Additional context used
🧬 Code graph analysis (5)
packages/run/src/__tests__/fixtures/netlify-adapter-not-edge/test.config.ts (1)
packages/run/src/__tests__/fixtures/netlify-adapter-edge/test.config.ts (1)
skip_preview(9-9)
packages/run/src/__tests__/fixtures/netlify-adapter-edge/test.config.ts (1)
packages/run/src/__tests__/fixtures/netlify-adapter-not-edge/test.config.ts (1)
skip_preview(9-9)
packages/run/src/__tests__/fixtures/force-tags-api/transform.ts (1)
packages/run/src/__tests__/fixtures/force-class-api/transform.ts (1)
Program(4-39)
packages/run/src/__tests__/fixtures/force-class-api/.marko-run/routes.d.ts (1)
packages/run/src/__tests__/fixtures/force-tags-api/.marko-run/routes.d.ts (9)
Route(21-21)Route(33-33)Context(22-22)Context(34-34)Handler(23-23)Handler(35-35)route(25-25)route(37-37)Input(30-30)
packages/run/src/vite/plugin.ts (2)
packages/run/src/vite/types.ts (1)
Route(87-97)packages/run/src/vite/codegen/index.ts (1)
renderRouteTemplate(37-63)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test: node@22 (windows-latest)
- GitHub Check: test: node@24 (ubuntu-latest)
- GitHub Check: test: node@24 (windows-latest)
- GitHub Check: test: node@20 (windows-latest)
- GitHub Check: test: node@20 (ubuntu-latest)
- GitHub Check: test: node@22 (ubuntu-latest)
🔇 Additional comments (12)
packages/run/src/vite/__tests__/fixtures/force-class-api/routes.txt (1)
1-2: LGTM!The test fixture correctly defines two routes: a layout with class API metadata and a page route. The JSON metadata syntax is valid and follows the expected format for the force-class-api test case.
packages/run/src/__tests__/fixtures/force-class-api/.marko-run/routes.d.ts (1)
1-39: Auto-generated type declarations look consistent and correct.The generated TypeScript declarations properly augment the Marko Run modules with route-specific types. The structure matches the force-tags-api fixture pattern, includes appropriate deprecation notices for the
routeconstant (lines 24-25, 36-37), and correctly defines the layout'sInputinterface (line 30). The type exports forRoute,Context, andHandlerare consistent across both page and layout modules.packages/run/src/vite/__tests__/fixtures/force-tags-api/routes.txt (1)
1-2: LGTM! Test fixture correctly defines routes with tags API.The route definitions are clear and appropriate for testing the forced tags API scenario.
packages/run/src/__tests__/fixtures/force-tags-api/.marko-run/routes.d.ts (1)
1-39: LGTM! Auto-generated type declarations are well-formed.The TypeScript declarations properly augment the @marko/run module and provide comprehensive type safety for the test fixture routes.
packages/run/src/__tests__/fixtures/force-tags-api/marko.json (1)
1-3: LGTM! Configuration correctly references the transformer.The Marko configuration properly links to the transform file for this test fixture.
.changeset/spicy-moles-stop.md (1)
1-5: Changeset looks correct and matches PR intent.packages/run/src/vite/plugin.ts (3)
300-304: WiringrenderRouteTemplate(route, await getMarkoApiForRoute(...))is the right direction.Also applies to: 319-323
695-695:virtualFiles.get(id)!is fine assumingvirtualFiles.has(id)remains the gate.
124-130: The SSR parameter concern is not applicable to Vite 7's per-environmenttransformRequestAPI. Theenvironmentproperty already knows its SSR mode, so metadata is recorded in the correct environment's module graph without a per-call parameter. The pattern of callingtransformRequest(id)thengetModuleInfo(id)aligns with the documented Vite 7 API.ID normalization may be worth checking if
route.layouts[0].filePathcan contain non-normalized paths (e.g., Windows backslashes), though the route builder likely already provides normalized paths. Additionally, thetemplateFilePath &&guard before accessinglayouts[0]is effective: the route builder only setstemplateFilePathwhencurrentLayouts.size > 0, ensuringlayouts[0]is always present.packages/run/src/vite/codegen/index.ts (3)
280-280:runtimeInclude?: stringsignature tweak looks fine.
37-63: TherenderRouteTemplatechange is correct and requires no escaping or whitelisting. ThemarkoApiparameter comes from the Marko compiler's trusted module metadata (module.meta?.markoAPI) and is limited to controlled values like"tags"or"class"(as verified by test fixtures). Since the injection target is an HTML comment, there is no injection vulnerability, and the implementation is stable.
792-795:route.path.pathis correctly used for type generation—it contains the decoded normalized route path.The route paths are derived from file system names which may contain percent-encoded characters (like
a%2fb%2fcfor/a/b/c), butroute.path.pathstores the decoded routing path suitable for TypeScript types. The runtime matching separately handles URL path decoding viadecodeURIComponentduring request handling. Using the raw path value directly is the intended behavior.
rturnq
force-pushed
the
route-entry-marko-api
branch
from
9431c17 to
b2dbca2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/run/src/vite/plugin.ts (1)
579-600: Marko API cache not invalidated on layout content changesThe dev watcher only clears
routeMarkoApiCachefor:
add/unlinkof any routable file, orchangeevents whereroutableFileTypeisHandlerorMiddleware, or the file isruntimeInclude.A
changeto a+layout.marko(which is exactly where themarkoAPImeta is sourced from) won’t reset the cache, sogetMarkoApiForRoutewill keep returning a stale API until some other qualifying file event occurs or the server restarts.You likely want layout edits to invalidate the cache as well. For example:
- (type === "change" && - (routableFileType === RoutableFileTypes.Handler || - routableFileType === RoutableFileTypes.Middleware || - filename === runtimeInclude)) + (type === "change" && + (routableFileType === RoutableFileTypes.Handler || + routableFileType === RoutableFileTypes.Middleware || + routableFileType === RoutableFileTypes.Layout || + filename === runtimeInclude)) ) { buildVirtualFilesResult = undefined; renderVirtualFilesResult = undefined; routeMarkoApiCache = undefined;so that editing a layout immediately forces Marko API re-resolution.
♻️ Duplicate comments (11)
packages/run/src/__tests__/fixtures/netlify-adapter-edge/test.config.ts (1)
9-9: Add semicolon before comment for style consistency.The previous review comment regarding the missing semicolon was marked as addressed, but the code still lacks a semicolon after
trueand before the//comment. For consistency with thenetlify-adapter-not-edgefixture and standard JavaScript/TypeScript style, add the semicolon.Apply this diff:
-export const skip_preview = true //process.env.CI === "true"; +export const skip_preview = true; //process.env.CI === "true";packages/run/src/__tests__/fixtures/force-tags-api/tsconfig.json (1)
1-4: Remove trailing comma to ensure strict JSON compatibility.The trailing comma after the
"include"array on line 3 makes this file invalid strict JSON, which may cause failures in tooling that doesn't support JSONC.Apply this diff:
{ "extends": "../../tsconfig-base.json", - "include": ["src/**/*", ".marko-run/*"], + "include": ["src/**/*", ".marko-run/*"] }packages/run/src/__tests__/fixtures/force-tags-api/transform.ts (1)
4-9: Guard against undefined filename to prevent TypeError.If
program.hub.file.opts.filenameis undefined, the regex.test()call on line 6-7 will throw. Add a null/undefined check before the regex test.Apply this diff:
export default { Program(program: t.NodePath<t.Program>) { + const filename = program.hub.file.opts.filename ?? ""; if ( /dist[\\/]\.marko-run[\\/].+(?<!entry)\.marko$/.test( - program.hub.file.opts.filename, + filename, ) ) {packages/run/src/__tests__/fixtures/netlify-adapter-edge/vite.config.ts (1)
7-9: HMR disabled consistently across fixtures.The change mirrors the pattern in other test fixtures. Same verification request applies regarding the rationale for disabling HMR.
packages/run/src/__tests__/fixtures/node-adapter-page/vite.config.ts (1)
7-9: HMR disabled consistently across fixtures.packages/run/src/__tests__/fixtures/static-adapter-file-html/vite.config.ts (1)
7-9: HMR disabled consistently across fixtures.packages/run/src/__tests__/fixtures/external-routes/vite.config.ts (1)
23-25: HMR disabled consistently across fixtures.packages/run/src/__tests__/fixtures/static-adapter-path-param/vite.config.ts (1)
7-9: HMR disabled consistently across fixtures.packages/run/src/__tests__/fixtures/inject-plugin-custom-entry/vite.config.ts (1)
8-10: HMR disabled consistently across fixtures.packages/run/package.json (1)
36-36: Confirm@marko/vite@^5.4.2exists (or adjust the range) before release.npm currently reports
@marko/vite@5.3.0as the latest published version and does not list5.4.2, so this dependency range may not be installable unless a matching release is cut in the@marko/viterepo before publishing@marko/run@0.9.3.(npmjs.com)Either (a) ensure
@marko/vite@5.4.2is published ahead of this release, or (b) relax this to a version range that matches an actually published version.packages/run/src/__tests__/fixtures/force-class-api/src/routes/+page.marko (1)
1-1: Optionally guard againstnullwhen checkingtypeof component === "object".If
componentcan ever benull,typeof component === "object"will treat it as"class"here. Consider tightening the check (e.g.component && typeof component === "object") unless the fixture guaranteescomponentis never null.
🧹 Nitpick comments (4)
packages/run/src/vite/__tests__/utils/fakeFS.ts (1)
1-1: Path construction is cross-platform, but consider usingpath.join()for robustness.The use of
sepfrom "path" correctly addresses cross-platform separator concerns raised in previous reviews. However, the string concatenation approach at line 51 can produce unexpected results in edge cases:
- When
baseDiris""(the default), paths get a leading separator (e.g.,/file.tsinstead offile.ts)- If
baseDirhas a trailing separator (e.g.,"src/routes/"), you get double separators (e.g.,src/routes//file.ts)Consider refactoring to use
path.join()for automatic normalization:-import { sep } from "path"; +import path from "path";} else { if (onData && data) { - onData(`${dirStack.join(sep)}${sep}${name}`, data); + onData(path.join(...dirStack, name), data); } current[1].push(name); }This handles empty
baseDirand trailing separators correctly across platforms.Also applies to: 50-52
packages/run/src/__tests__/main.test.ts (2)
142-142: Using a shareddefault.config.tsviabaseViteConfigFileis a good test simplification.Passing
baseViteConfigFileintocli.getViteConfigcentralizes common Vite config across fixtures and should reduce duplication; just ensure other call sites ofgetViteConfigwere updated consistently with the new parameter ordering.Also applies to: 194-199, 216-221
273-273: Commented‑outwaitForLoadStatedeserves a short rationale or cleanup.If this was disabled due to flakiness and is no longer needed, consider either removing it entirely or adding a brief comment explaining why it must stay disabled so future readers don’t re‑enable it accidentally.
packages/run/src/vite/codegen/index.ts (1)
792-795: Consider using JSON.stringify for path type literalsBuilding
pathTypeas a raw interpolated string assumesroute.path.pathnever contains characters that need escaping inside a TS string literal.You could make this more robust by letting JS handle escaping:
- const pathType = `"${route.path.path}"`; + const pathType = JSON.stringify(route.path.path);This keeps behavior the same for normal paths while hardening against any unexpected characters.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (39)
package-lock.jsonis excluded by!**/package-lock.jsonand included by**packages/run/src/__tests__/fixtures/force-class-api/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/force-class-api/__snapshots__/preview.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/force-tags-api/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/force-tags-api/__snapshots__/preview.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/basic-500/__snapshots__/basic-500.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/basic-500/__snapshots__/basic-500.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/basic/__snapshots__/basic.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/basic/__snapshots__/basic.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/build-routes/__snapshots__/build-routes.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/build-routes/__snapshots__/build-routes.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/build-routes/__snapshots__/build-routes.expected.routetypes.d.tsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/escape-path/__snapshots__/escape-path.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/escape-path/__snapshots__/escape-path.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/flat-routes/__snapshots__/flat-routes.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/flat-routes/__snapshots__/flat-routes.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/flat-routes/__snapshots__/flat-routes.expected.routetypes.d.tsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-class-api/__snapshots__/force-class-api.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-class-api/__snapshots__/force-class-api.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-class-api/__snapshots__/force-class-api.expected.routetypes.d.tsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-tags-api/__snapshots__/force-tags-api.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-tags-api/__snapshots__/force-tags-api.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-tags-api/__snapshots__/force-tags-api.expected.routetypes.d.tsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/multiple-sources/__snapshots__/multiple-sources.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/multiple-sources/__snapshots__/multiple-sources.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/nested-dynamic/__snapshots__/nested-dynamic.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/nested-dynamic/__snapshots__/nested-dynamic.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/nested-dynamic/__snapshots__/nested-dynamic.expected.routetypes.d.tsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/optional-dynamic/__snapshots__/optional-dynamic.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/optional-dynamic/__snapshots__/optional-dynamic.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/optional-static/__snapshots__/optional-static.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/optional-static/__snapshots__/optional-static.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/optional-types/__snapshots__/optional-types.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/optional-types/__snapshots__/optional-types.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/optional-types/__snapshots__/optional-types.expected.routetypes.d.tsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/param-optional-rest/__snapshots__/param-optional-rest.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/param-optional-rest/__snapshots__/param-optional-rest.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/uri-encoded/__snapshots__/uri-encoded.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/uri-encoded/__snapshots__/uri-encoded.expected.routes.mdis excluded by!**/__snapshots__/**and included by**
📒 Files selected for processing (67)
.changeset/spicy-moles-stop.md(1 hunks).github/workflows/ci.yml(3 hunks)examples/netlify/package.json(1 hunks)examples/node-express/package.json(1 hunks)examples/static/package.json(1 hunks)examples/zero-config/package.json(1 hunks)package.json(1 hunks)packages/explorer/package.json(1 hunks)packages/run/package.json(1 hunks)packages/run/src/__tests__/default.config.ts(1 hunks)packages/run/src/__tests__/fixtures/adapter-is-entry/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/basic-assets/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/config-override/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/config-port-override/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/external-routes/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/.marko-run/routes.d.ts(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/marko.json(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/src/routes/+layout.marko(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/src/routes/+page.marko(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/test.config.ts(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/transform.ts(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/tsconfig.json(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/.marko-run/routes.d.ts(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/marko.json(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/src/routes/+layout.marko(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/src/routes/+page.marko(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/test.config.ts(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/transform.ts(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/tsconfig.json(1 hunks)packages/run/src/__tests__/fixtures/inject-plugin-custom-entry/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/inject-plugin/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/netlify-adapter-edge/netlify.toml(1 hunks)packages/run/src/__tests__/fixtures/netlify-adapter-edge/test.config.ts(1 hunks)packages/run/src/__tests__/fixtures/netlify-adapter-edge/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/netlify-adapter-not-edge/test.config.ts(1 hunks)packages/run/src/__tests__/fixtures/netlify-adapter-not-edge/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/no-routes/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/node-adapter-express-bodyParser/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/node-adapter-express-formData/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/node-adapter-express-import/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/node-adapter-express-match/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/node-adapter-express/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/node-adapter-page/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/runtime-include/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/static-adapter-file-html/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/static-adapter-file-plain/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/static-adapter-invalid-url/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/static-adapter-page/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/static-adapter-path-param/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/static-adapter-redirect/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/static-adapter-rest-param/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/static-adapter-trailing-slash/vite.config.ts(1 hunks)packages/run/src/__tests__/main.test.ts(5 hunks)packages/run/src/cli/commands.ts(2 hunks)packages/run/src/vite/__tests__/fixtures/build-routes/routes.txt(2 hunks)packages/run/src/vite/__tests__/fixtures/flat-routes/routes.txt(1 hunks)packages/run/src/vite/__tests__/fixtures/force-class-api/routes.txt(1 hunks)packages/run/src/vite/__tests__/fixtures/force-tags-api/routes.txt(1 hunks)packages/run/src/vite/__tests__/fixtures/nested-dynamic/routes.txt(1 hunks)packages/run/src/vite/__tests__/fixtures/optional-types/routes.txt(1 hunks)packages/run/src/vite/__tests__/main.test.ts(5 hunks)packages/run/src/vite/__tests__/route-builder.test.ts(1 hunks)packages/run/src/vite/__tests__/utils/fakeFS.ts(2 hunks)packages/run/src/vite/codegen/index.ts(3 hunks)packages/run/src/vite/plugin.ts(6 hunks)packages/run/src/vite/routes/walk.ts(2 hunks)packages/run/src/vite/utils/log.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (24)
- packages/run/src/tests/fixtures/force-tags-api/marko.json
- packages/run/src/tests/fixtures/force-tags-api/src/routes/+page.marko
- packages/run/src/tests/fixtures/force-class-api/tsconfig.json
- packages/run/src/vite/utils/log.ts
- examples/netlify/package.json
- examples/static/package.json
- examples/zero-config/package.json
- packages/run/src/vite/tests/fixtures/force-tags-api/routes.txt
- packages/run/src/tests/fixtures/netlify-adapter-not-edge/test.config.ts
- packages/run/src/vite/tests/main.test.ts
- packages/run/src/tests/fixtures/force-class-api/transform.ts
- packages/run/src/tests/fixtures/force-tags-api/src/routes/+layout.marko
- packages/run/src/tests/fixtures/force-class-api/marko.json
- packages/run/src/tests/fixtures/force-class-api/src/routes/+layout.marko
- packages/run/src/tests/fixtures/netlify-adapter-edge/netlify.toml
- packages/run/src/vite/tests/fixtures/optional-types/routes.txt
- packages/run/src/tests/fixtures/force-tags-api/.marko-run/routes.d.ts
- packages/run/src/vite/tests/fixtures/force-class-api/routes.txt
- packages/run/src/tests/fixtures/force-class-api/test.config.ts
- examples/node-express/package.json
- packages/run/src/vite/tests/route-builder.test.ts
- packages/explorer/package.json
- packages/run/src/vite/tests/fixtures/build-routes/routes.txt
- packages/run/src/vite/tests/fixtures/nested-dynamic/routes.txt
🧰 Additional context used
🧬 Code graph analysis (7)
packages/run/src/__tests__/fixtures/force-tags-api/transform.ts (1)
packages/run/src/__tests__/fixtures/force-class-api/transform.ts (1)
Program(4-39)
packages/run/src/cli/commands.ts (1)
packages/run/src/adapter/default-entry.mjs (1)
__dirname(9-9)
packages/run/src/vite/__tests__/utils/fakeFS.ts (1)
packages/run/src/vite/routes/walk.ts (1)
TestFileTree(68-68)
packages/run/src/__tests__/fixtures/force-tags-api/test.config.ts (1)
packages/run/src/__tests__/fixtures/force-class-api/test.config.ts (1)
steps(1-1)
packages/run/src/__tests__/fixtures/force-class-api/.marko-run/routes.d.ts (1)
packages/run/src/vite/__tests__/fixtures/force-class-api/__snapshots__/force-class-api.expected.routetypes.d.ts (9)
Route(21-21)Route(33-33)Context(22-22)Context(34-34)Handler(23-23)Handler(35-35)route(25-25)route(37-37)Input(30-30)
packages/run/src/vite/plugin.ts (2)
packages/run/src/vite/types.ts (1)
Route(87-97)packages/run/src/vite/codegen/index.ts (1)
renderRouteTemplate(37-63)
packages/run/src/__tests__/fixtures/netlify-adapter-edge/test.config.ts (1)
packages/run/src/__tests__/fixtures/netlify-adapter-not-edge/test.config.ts (1)
skip_preview(9-9)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test: node@24 (windows-latest)
- GitHub Check: test: node@20 (windows-latest)
- GitHub Check: test: node@22 (windows-latest)
- GitHub Check: test: node@24 (ubuntu-latest)
- GitHub Check: test: node@20 (ubuntu-latest)
- GitHub Check: test: node@22 (ubuntu-latest)
🔇 Additional comments (32)
packages/run/src/__tests__/fixtures/force-tags-api/test.config.ts (1)
1-1: LGTM!Test fixture configuration with empty steps array is appropriate for this test setup.
packages/run/src/cli/commands.ts (2)
268-268: LGTM!The return statement correctly uses the new
fallbackparameter, making the fallback path configurable while preserving the default behavior.
247-252: Good addition for configurable fallback behavior.The new
fallbackparameter maintains backward compatibility with its default value and enables customizable fallback config resolution. Thedefault.config.mjsfile exists at the expected location in the same directory as the commands module.packages/run/src/__tests__/fixtures/static-adapter-redirect/vite.config.ts (1)
7-9: Verify the rationale for disabling HMR in test fixtures.The addition of
server: { hmr: false }is consistent with the pattern applied across multiple test fixtures in this PR. However, the reasoning for disabling Hot Module Replacement in test configurations is not documented.Please confirm: Is this necessary to ensure test determinism, avoid HMR-related flakiness, or test production-like behavior? Consider adding a comment explaining the rationale for future maintainers.
.changeset/spicy-moles-stop.md (1)
1-5: The changeset correctly classifies this as a patch release.renderRouteTemplateandrenderRouterare not part of the public API surface—they are internal utilities exported only fromcodegen/index.tsfor use within the vite plugin implementation (as seen inplugin.tsand tests). The vite entry point (src/vite/index.ts) does not re-export these functions, so they are not exposed to package consumers. Internal API changes do not require version bumps.packages/run/src/__tests__/fixtures/node-adapter-express-formData/vite.config.ts (1)
7-9: LGTM!Disabling HMR in test fixtures ensures deterministic test behavior.
packages/run/src/__tests__/fixtures/node-adapter-express-bodyParser/vite.config.ts (1)
7-9: LGTM!Consistent with the pattern of disabling HMR across test fixtures for stable test environments.
packages/run/src/__tests__/fixtures/config-port-override/vite.config.ts (1)
6-6: LGTM!The HMR disable is correctly added to the existing server configuration.
packages/run/src/__tests__/fixtures/runtime-include/vite.config.ts (1)
8-10: LGTM!Appropriate test configuration for consistent test execution.
packages/run/src/__tests__/fixtures/basic-assets/vite.config.ts (1)
7-9: LGTM!The server configuration is correctly added alongside the existing build configuration.
packages/run/src/__tests__/fixtures/adapter-is-entry/vite.config.ts (1)
8-10: LGTM!Consistent HMR disabling for test fixture stability.
packages/run/src/__tests__/fixtures/inject-plugin/vite.config.ts (1)
8-10: LGTM!The change aligns with the test suite's approach to controlling dev-server behavior.
packages/run/src/__tests__/fixtures/static-adapter-invalid-url/vite.config.ts (1)
7-9: LGTM!The HMR disabling is correctly configured for this test fixture.
packages/run/src/__tests__/fixtures/static-adapter-file-plain/vite.config.ts (1)
7-9: HMR disablement in this fixture looks good.
server.hmr: falseis valid Vite config and matches the pattern used across the other fixtures for more deterministic tests.packages/run/src/__tests__/fixtures/config-override/vite.config.ts (1)
4-6: Config-override fixture server settings are reasonable.Adding
server.hmr: falseis consistent with the other fixtures and should not interfere with config-override semantics.packages/run/src/__tests__/fixtures/no-routes/vite.config.ts (1)
7-9: Disabling HMR in the no-routes fixture is consistent with the rest of the suite.This should help keep dev server behavior stable for these tests.
packages/run/src/__tests__/fixtures/static-adapter-rest-param/vite.config.ts (1)
7-9: Static rest-param fixture HMR config looks correct.The new
server.hmr: falseproperty is syntactically valid and aligns this fixture with the others.packages/run/src/__tests__/fixtures/static-adapter-page/vite.config.ts (1)
7-9: HMR disabled for static page fixture is appropriate.The new
serverblock is straightforward and consistent with other static adapter configs.packages/run/src/__tests__/fixtures/static-adapter-trailing-slash/vite.config.ts (1)
10-12: Trailing-slash fixture config remains valid with HMR turned off.
server.hmr: falseis added cleanly after the existing plugin options and should not affect the trailing-slash behavior under test.packages/run/src/vite/__tests__/fixtures/flat-routes/routes.txt (1)
2-2: Route metadata change toverbsarray looks correct.Switching to
handler.markowith{ "verbs": ["get", "post"] }matches the newer metadata-driven routing convention and keeps the semantics of supporting both GET and POST.packages/run/src/__tests__/fixtures/netlify-adapter-not-edge/vite.config.ts (1)
7-9: Disabling HMR in this fixture looks correct.Aligns with other fixtures and should reduce dev/HMR noise in tests without affecting build behavior.
packages/run/src/__tests__/fixtures/node-adapter-express-match/vite.config.ts (1)
7-9: Consistent HMR disabling for this fixture.Matches other Vite test configs and should stabilize snapshots without impacting non-dev behavior.
packages/run/src/__tests__/default.config.ts (1)
1-9: Shared default Vite config for tests looks good.Centralizing the Marko plugin +
server.hmr: falseintodefault.config.tsshould simplify fixtures and keep behavior consistent across tests.packages/run/src/__tests__/fixtures/node-adapter-express-import/vite.config.ts (1)
7-9: HMR disabled consistently across node/express fixtures.This matches other updated fixtures and should avoid extra Vite client noise in test snapshots.
.github/workflows/ci.yml (1)
18-23: CI action and Node matrix upgrades look reasonable; confirm external compatibility.Moving to
actions/checkout@v6,actions/setup-node@v6, and adding Node 24 to build/test/release is a sensible modernization, and the explicitreleasepermissions are appropriate. Please just double‑check that the v6 actions and your release tooling fully support Node 24 in GitHub Actions for this repo’s target environments.Also applies to: 37-43, 60-63, 66-70
packages/run/src/__tests__/main.test.ts (1)
5-5: Direct default import ofmocha-snapis cleaner; confirm export shape.Switching to
import snap from "mocha-snap";removes the extra indirection; as long as your TS/ESM interop matches mocha‑snap’s default export, this is preferable.packages/run/src/vite/routes/walk.ts (1)
70-109:createTestWalkersimplification looks sound; ensure callers pass the desired root path.The new API and traversal mirror
createFSWalkermore closely and keep the logic localized tocurrentPath. Just make sure allTestFileTreebuilders now store the intended root path indir[0]so that thepathvalues seen byonEnter/onFilestay consistent with the real FS walker.package.json (1)
25-65: marko@^5.38.2 now valid; dependency bumps look reasonableThe previous concern about
marko@^5.38.2not existing on npm is obsolete;5.38.2is currently published and listed as the latest 5.x. The updated devDependency ranges (including TypeScript/eslint/Vite) look consistent with the rest of the PR; just ensure CI passes across the supported Node matrix.packages/run/src/__tests__/fixtures/force-class-api/.marko-run/routes.d.ts (1)
1-39: Generated route typings align with codegen outputThe AppData and per-module
MarkoRunnamespaces (includingInputfor+layout.marko) match the structure produced byrenderRouteTypeInfoandwriteModuleDeclaration; this fixture looks correct and consistent with the expected snapshots.packages/run/src/vite/codegen/index.ts (1)
37-63: Route template rendering with optional Marko API directive looks correctThe new
renderRouteTemplate(route, markoApi?)flow correctly validatespage/templateFilePath, optionally emits the<!-- use ${markoApi} -->directive, and still useswriteEntryTemplateTagover normalized relative layout/page paths. This should integrate cleanly with the per-route Marko API resolution in the plugin.packages/run/src/vite/plugin.ts (1)
690-702: Non-null assertion on virtualFiles.get(id) is safe in current control flowThe
loadhook now does:if (virtualFiles.has(id)) { return virtualFiles.get(id)!; }Given the guarded
has(id)check and thatvirtualFilesonly stores concrete strings (including""), this looks fine and keeps TS happy without changing behavior.packages/run/src/__tests__/fixtures/node-adapter-express/vite.config.ts (1)
5-10: Disabling HMR in test fixture Vite config makes sense
server.hmr: falsefor this fixture is appropriate to keep test runs deterministic and avoid HMR-related noise during integration tests.
rturnq
force-pushed
the
route-entry-marko-api
branch
from
b2dbca2 to
14f5943
Compare
rturnq
force-pushed
the
route-entry-marko-api
branch
from
14f5943 to
14a332a
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (8)
packages/run/src/vite/utils/log.ts (1)
185-187: The past review comment regarding Blob allocation remains unaddressed.The addition of
as BlobPartcast suppresses the type error but doesn't resolve the underlying performance concern. TheBloballocation approach is still inefficient compared to direct byte-length checks.As suggested in the previous review, consider this more efficient implementation:
function byteSize(source: string | Uint8Array): number { - return new Blob([source as BlobPart]).size; + return typeof source === "string" ? Buffer.byteLength(source) : source.byteLength; }packages/run/src/__tests__/fixtures/netlify-adapter-not-edge/test.config.ts (1)
9-9: Hard-codingskip_preview = truepermanently disables preview tests.This concern was already raised in a previous review. The commented-out condition
//process.env.CI === "true"suggests this was recently changed from environment-based to always-true. Sincepreview.expected.mdandpreview_argsstill exist in this fixture, verify that permanently disabling preview tests is intentional for these Netlify adapter fixtures, or restore the conditional logic.packages/run/src/__tests__/fixtures/netlify-adapter-edge/test.config.ts (1)
9-9: Add missing semicolon for consistency and verify hard-coded preview skip.Line 9 is missing a semicolon after
true, creating style inconsistency with thenetlify-adapter-not-edgefixture which usestrue; //. Additionally, hard-codingskip_preview = truepermanently disables preview tests—confirm this is intentional despite the commented-out conditional.Apply this diff to align the style:
-export const skip_preview = true //process.env.CI === "true"; +export const skip_preview = true; //process.env.CI === "true";packages/run/src/__tests__/fixtures/force-tags-api/transform.ts (1)
5-9: Guard against undefinedfilenameto prevent potential TypeError.The same issue exists here as in the force-class-api transform: the regex test will throw if
program.hub.file.opts.filenameis undefined.Apply this diff to add a safety guard:
Program(program: t.NodePath<t.Program>) { + const filename = program.hub.file.opts.filename ?? ""; if ( /dist[\\/]\.marko-run[\\/].+(?<!entry)\.marko$/.test( - program.hub.file.opts.filename, + filename, ) ) {packages/run/src/vite/__tests__/utils/fakeFS.ts (1)
50-52: Path construction fails whenbaseDiris empty.The current string concatenation
${dirStack.join(sep)}${sep}${name}produces an incorrect path whenbaseDiris""(the default). For example, withdirStack = [""]andname = "file.txt", the result is/file.txton Unix instead offile.txt.Use
path.joinfor cleaner and correct path construction across all cases.Apply this diff to fix the path construction:
-import { sep } from "path"; +import { join } from "path";} else { if (onData && data) { - onData(`${dirStack.join(sep)}${sep}${name}`, data); + onData(join(...dirStack, name), data); } current[1].push(name); }packages/run/package.json (1)
36-36: [Duplicate] Version@marko/vite@^5.4.2does not exist on npm.This issue was already flagged in a previous review. The npm registry shows version 5.3.0 as the latest published version of
@marko/vite. Please correct this to a valid published version.examples/node-express/package.json (1)
14-14: [Duplicate] Versionmarko@^5.38.2does not exist on npm.This issue was already flagged in a previous review. The npm registry shows 5.37.55 as the latest 5.x release. Please update to a valid published version.
package.json (1)
52-52: [Duplicate] Versionmarko@^5.38.2does not exist on npm.This issue was already flagged in a previous review. The npm registry shows 5.37.x as the latest published 5.x series. Please update to a valid published version.
🧹 Nitpick comments (1)
packages/run/src/vite/utils/server.ts (1)
114-124: Consider documenting the return value and timeout behavior.The
waitForExithelper returns the exit code on success ornullon timeout. This behavior should be documented to clarify the semantics, especially since exit code0is a valid success value.Apply this diff to add JSDoc:
+/** + * Waits for a child process to exit. + * @param proc - The child process to wait for + * @param wait - Timeout in milliseconds (0 = wait indefinitely) + * @returns The exit code if the process exits, or null if timeout is reached + */ async function waitForExit(proc: ChildProcess, wait: number = 0) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (39)
package-lock.jsonis excluded by!**/package-lock.jsonand included by**packages/run/src/__tests__/fixtures/force-class-api/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/force-class-api/__snapshots__/preview.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/force-tags-api/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/force-tags-api/__snapshots__/preview.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/basic-500/__snapshots__/basic-500.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/basic-500/__snapshots__/basic-500.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/basic/__snapshots__/basic.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/basic/__snapshots__/basic.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/build-routes/__snapshots__/build-routes.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/build-routes/__snapshots__/build-routes.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/build-routes/__snapshots__/build-routes.expected.routetypes.d.tsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/escape-path/__snapshots__/escape-path.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/escape-path/__snapshots__/escape-path.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/flat-routes/__snapshots__/flat-routes.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/flat-routes/__snapshots__/flat-routes.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/flat-routes/__snapshots__/flat-routes.expected.routetypes.d.tsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-class-api/__snapshots__/force-class-api.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-class-api/__snapshots__/force-class-api.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-class-api/__snapshots__/force-class-api.expected.routetypes.d.tsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-tags-api/__snapshots__/force-tags-api.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-tags-api/__snapshots__/force-tags-api.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-tags-api/__snapshots__/force-tags-api.expected.routetypes.d.tsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/multiple-sources/__snapshots__/multiple-sources.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/multiple-sources/__snapshots__/multiple-sources.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/nested-dynamic/__snapshots__/nested-dynamic.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/nested-dynamic/__snapshots__/nested-dynamic.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/nested-dynamic/__snapshots__/nested-dynamic.expected.routetypes.d.tsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/optional-dynamic/__snapshots__/optional-dynamic.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/optional-dynamic/__snapshots__/optional-dynamic.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/optional-static/__snapshots__/optional-static.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/optional-static/__snapshots__/optional-static.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/optional-types/__snapshots__/optional-types.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/optional-types/__snapshots__/optional-types.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/optional-types/__snapshots__/optional-types.expected.routetypes.d.tsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/param-optional-rest/__snapshots__/param-optional-rest.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/param-optional-rest/__snapshots__/param-optional-rest.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/uri-encoded/__snapshots__/uri-encoded.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/uri-encoded/__snapshots__/uri-encoded.expected.routes.mdis excluded by!**/__snapshots__/**and included by**
📒 Files selected for processing (68)
.changeset/spicy-moles-stop.md(1 hunks).github/workflows/ci.yml(3 hunks)examples/netlify/package.json(1 hunks)examples/node-express/package.json(1 hunks)examples/static/package.json(1 hunks)examples/zero-config/package.json(1 hunks)package.json(1 hunks)packages/explorer/package.json(1 hunks)packages/run/package.json(1 hunks)packages/run/src/__tests__/default.config.ts(1 hunks)packages/run/src/__tests__/fixtures/adapter-is-entry/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/basic-assets/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/config-override/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/config-port-override/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/external-routes/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/.marko-run/routes.d.ts(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/marko.json(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/src/routes/+layout.marko(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/src/routes/+page.marko(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/test.config.ts(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/transform.ts(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/tsconfig.json(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/.marko-run/routes.d.ts(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/marko.json(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/src/routes/+layout.marko(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/src/routes/+page.marko(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/test.config.ts(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/transform.ts(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/tsconfig.json(1 hunks)packages/run/src/__tests__/fixtures/inject-plugin-custom-entry/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/inject-plugin/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/netlify-adapter-edge/netlify.toml(1 hunks)packages/run/src/__tests__/fixtures/netlify-adapter-edge/test.config.ts(1 hunks)packages/run/src/__tests__/fixtures/netlify-adapter-edge/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/netlify-adapter-not-edge/test.config.ts(1 hunks)packages/run/src/__tests__/fixtures/netlify-adapter-not-edge/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/no-routes/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/node-adapter-express-bodyParser/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/node-adapter-express-formData/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/node-adapter-express-import/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/node-adapter-express-match/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/node-adapter-express/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/node-adapter-page/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/runtime-include/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/static-adapter-file-html/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/static-adapter-file-plain/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/static-adapter-invalid-url/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/static-adapter-page/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/static-adapter-path-param/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/static-adapter-redirect/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/static-adapter-rest-param/vite.config.ts(1 hunks)packages/run/src/__tests__/fixtures/static-adapter-trailing-slash/vite.config.ts(1 hunks)packages/run/src/__tests__/main.test.ts(5 hunks)packages/run/src/cli/commands.ts(2 hunks)packages/run/src/vite/__tests__/fixtures/build-routes/routes.txt(2 hunks)packages/run/src/vite/__tests__/fixtures/flat-routes/routes.txt(1 hunks)packages/run/src/vite/__tests__/fixtures/force-class-api/routes.txt(1 hunks)packages/run/src/vite/__tests__/fixtures/force-tags-api/routes.txt(1 hunks)packages/run/src/vite/__tests__/fixtures/nested-dynamic/routes.txt(1 hunks)packages/run/src/vite/__tests__/fixtures/optional-types/routes.txt(1 hunks)packages/run/src/vite/__tests__/main.test.ts(5 hunks)packages/run/src/vite/__tests__/route-builder.test.ts(1 hunks)packages/run/src/vite/__tests__/utils/fakeFS.ts(2 hunks)packages/run/src/vite/codegen/index.ts(3 hunks)packages/run/src/vite/plugin.ts(6 hunks)packages/run/src/vite/routes/walk.ts(2 hunks)packages/run/src/vite/utils/log.ts(1 hunks)packages/run/src/vite/utils/server.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/run/src/tests/fixtures/force-tags-api/src/routes/+layout.marko
🚧 Files skipped from review as they are similar to previous changes (42)
- packages/run/src/tests/fixtures/force-tags-api/src/routes/+page.marko
- packages/run/src/tests/fixtures/external-routes/vite.config.ts
- packages/run/src/tests/fixtures/static-adapter-invalid-url/vite.config.ts
- packages/run/src/tests/fixtures/force-tags-api/test.config.ts
- packages/run/src/tests/fixtures/force-tags-api/tsconfig.json
- packages/run/src/tests/fixtures/force-class-api/marko.json
- packages/run/src/tests/fixtures/force-class-api/tsconfig.json
- .github/workflows/ci.yml
- packages/run/src/tests/fixtures/node-adapter-express/vite.config.ts
- packages/run/src/vite/tests/fixtures/force-tags-api/routes.txt
- examples/static/package.json
- packages/run/src/tests/fixtures/netlify-adapter-edge/netlify.toml
- examples/zero-config/package.json
- packages/run/src/tests/fixtures/node-adapter-express-formData/vite.config.ts
- packages/run/src/tests/default.config.ts
- packages/run/src/tests/fixtures/basic-assets/vite.config.ts
- packages/run/src/tests/fixtures/static-adapter-file-plain/vite.config.ts
- packages/run/src/tests/fixtures/static-adapter-page/vite.config.ts
- packages/run/src/vite/plugin.ts
- packages/run/src/tests/fixtures/static-adapter-rest-param/vite.config.ts
- packages/run/src/tests/fixtures/node-adapter-express-import/vite.config.ts
- examples/netlify/package.json
- packages/run/src/vite/tests/fixtures/nested-dynamic/routes.txt
- packages/run/src/tests/fixtures/config-override/vite.config.ts
- packages/run/src/vite/tests/fixtures/force-class-api/routes.txt
- packages/run/src/tests/fixtures/node-adapter-page/vite.config.ts
- packages/run/src/tests/fixtures/config-port-override/vite.config.ts
- packages/run/src/tests/fixtures/no-routes/vite.config.ts
- packages/run/src/tests/fixtures/static-adapter-redirect/vite.config.ts
- packages/run/src/vite/tests/fixtures/optional-types/routes.txt
- .changeset/spicy-moles-stop.md
- packages/run/src/tests/fixtures/static-adapter-file-html/vite.config.ts
- packages/run/src/tests/fixtures/node-adapter-express-match/vite.config.ts
- packages/run/src/tests/fixtures/runtime-include/vite.config.ts
- packages/run/src/vite/tests/main.test.ts
- packages/run/src/tests/fixtures/force-class-api/.marko-run/routes.d.ts
- packages/run/src/tests/fixtures/adapter-is-entry/vite.config.ts
- packages/run/src/vite/tests/fixtures/flat-routes/routes.txt
- packages/run/src/tests/fixtures/inject-plugin/vite.config.ts
- packages/run/src/tests/fixtures/netlify-adapter-edge/vite.config.ts
- packages/run/src/tests/fixtures/force-class-api/src/routes/+page.marko
- packages/run/src/tests/fixtures/force-class-api/test.config.ts
🧰 Additional context used
🧬 Code graph analysis (7)
packages/run/src/__tests__/fixtures/force-class-api/transform.ts (1)
packages/run/src/__tests__/fixtures/force-tags-api/transform.ts (1)
Program(4-39)
packages/run/src/__tests__/fixtures/netlify-adapter-not-edge/test.config.ts (1)
packages/run/src/__tests__/fixtures/netlify-adapter-edge/test.config.ts (1)
skip_preview(9-9)
packages/run/src/vite/__tests__/utils/fakeFS.ts (1)
packages/run/src/vite/routes/walk.ts (1)
TestFileTree(68-68)
packages/run/src/__tests__/fixtures/netlify-adapter-edge/test.config.ts (1)
packages/run/src/__tests__/fixtures/netlify-adapter-not-edge/test.config.ts (1)
skip_preview(9-9)
packages/run/src/vite/__tests__/route-builder.test.ts (2)
packages/run/src/vite/routes/walk.ts (1)
createTestWalker(70-110)packages/run/src/vite/__tests__/utils/fakeFS.ts (1)
createDirectory(5-62)
packages/run/src/vite/codegen/index.ts (1)
packages/run/src/vite/codegen/writer.ts (1)
createStringWriter(165-179)
packages/run/src/cli/commands.ts (1)
packages/run/src/adapter/default-entry.mjs (1)
__dirname(9-9)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test: node@22 (ubuntu-latest)
- GitHub Check: test: node@22 (windows-latest)
- GitHub Check: test: node@24 (windows-latest)
- GitHub Check: test: node@20 (ubuntu-latest)
- GitHub Check: test: node@24 (ubuntu-latest)
- GitHub Check: test: node@20 (windows-latest)
🔇 Additional comments (29)
packages/run/src/__tests__/fixtures/static-adapter-trailing-slash/vite.config.ts (1)
10-12: LGTM! Disabling HMR improves test reliability.Disabling hot module replacement in test fixtures is a good practice—it makes tests more deterministic by avoiding file-watching race conditions and unnecessary rebuilds during test execution.
packages/run/src/__tests__/fixtures/inject-plugin-custom-entry/vite.config.ts (1)
8-10: LGTM! Disabling HMR for test determinism.Disabling HMR in test fixtures is a good practice to ensure deterministic test behavior and avoid timing-related flakiness.
packages/run/src/__tests__/fixtures/netlify-adapter-not-edge/vite.config.ts (1)
7-9: LGTM! Improves test determinism.Disabling HMR in test fixtures is a good practice to ensure consistent, deterministic behavior during automated testing. This prevents potential race conditions and timing issues that HMR might introduce.
packages/run/src/vite/__tests__/fixtures/build-routes/routes.txt (3)
4-16: Generic handler + verbs metadata looks consistent for_protected/_home.Using a shared
+handler.tswith explicit["post"]and["put","post","delete"]verb metadata, plus the added+layout.markoand+middleware.ts, matches the new per-route API shape and keeps the fixture readable. I don’t see any missing verbs or obvious mismatches in this subtree.
21-23: Intentional difference in HEAD support between/callback/oauth2and/my?
/callback/oauth2only advertises["get"], while/myexposes["get","head"]. If HEAD handling is supposed to be implicit for GET routes in this new API, you might want consistency here; if it’s explicitly opt‑in, then this distinction makes sense but is worth double‑checking against the router behavior.
26-30: New root‑level error/layout routes align with top‑level layout API.Adding
+404.marko,+500.marko,+layout.marko, and+middleware.tsat the root fits the PR’s “top layout for route entry templates” goal and gives good coverage in this fixture. Just ensure any tests that assert the full route list are updated to include these new entries.packages/run/src/__tests__/fixtures/static-adapter-path-param/vite.config.ts (1)
7-9: LGTM! Disabling HMR for test stability.Disabling Hot Module Replacement in test fixtures is a sound practice to ensure deterministic test behavior and avoid race conditions from hot reloading during tests.
packages/run/src/vite/codegen/index.ts (3)
37-63: LGTM! Per-route Marko API support implemented correctly.The updated signature and implementation correctly support per-route Marko API resolution. The conditional HTML comment emission when
markoApiis provided is a clean way to communicate API preferences to the Marko compiler.
280-280: LGTM! MakingruntimeIncludeoptional is sensible.This parameter is not always required, so making it optional improves the API ergonomics.
792-792: LGTM! Simplified path type generation.Directly using
route.path.pathis clearer and removes the dependency on the now-removeddecodePathhelper.packages/run/src/vite/__tests__/route-builder.test.ts (1)
13-13: LGTM! Test updated correctly for new walker API.The change correctly adapts to the updated
createTestWalkerandcreateDirectorysignatures, where the base directory is now specified increateDirectoryrather than as a separate parameter tocreateTestWalker.packages/run/src/vite/routes/walk.ts (1)
70-109: LGTM! API simplification improves clarity.The removal of the
rootparameter simplifies the API while maintaining correct path handling. The walker now computes paths directly from the tree structure viacurrentPath, which is more intuitive and eliminates redundant path construction logic.packages/run/src/cli/commands.ts (2)
268-268: LGTM!The return statement correctly uses the parameterized
fallbackvalue, making the function more flexible and maintainable.
251-251: Good parameterization for flexibility and backward compatibility.The addition of the
fallbackparameter with a default value allows callers to override the default config path while maintaining backward compatibility. The default path correctly points topackages/run/src/cli/default.config.mjs, which exists and will be resolved properly at runtime.packages/run/src/__tests__/fixtures/force-class-api/src/routes/+layout.marko (1)
1-14: LGTM! Clean test fixture for API mode verification.The layout template correctly implements diagnostic logic to distinguish between class and tags API modes, which aligns with the PR's objective to use Marko API of top layout for route entry templates.
packages/run/src/__tests__/fixtures/force-tags-api/.marko-run/routes.d.ts (1)
1-39: LGTM! Auto-generated type declarations are correctly structured.The TypeScript module augmentations properly define route types, context types, and handler types for the force-tags-api test fixture. The file structure follows the expected pattern for Marko Run type generation.
packages/run/src/__tests__/fixtures/force-tags-api/marko.json (1)
1-3: LGTM! Configuration correctly references the transformer.The Marko configuration properly maps the custom transform for this test fixture.
packages/run/src/__tests__/main.test.ts (2)
5-5: LGTM!The import simplification is cleaner and eliminates the need for the local redefinition that was previously required.
142-142: ThegetViteConfigsignature correctly accepts the fourth parameter.The function definition in
packages/run/src/cli/commands.tsacceptsfallback: string = path.join(__dirname, "default.config.mjs")as its fourth parameter. The test correctly passesbaseViteConfigFileas this argument on lines 194–199 and 216–221, and the referenced filepackages/run/src/__tests__/default.config.tsexists.packages/run/src/vite/__tests__/utils/fakeFS.ts (7)
1-1: Import looks correct for current usage.The
sepimport is appropriate for the path construction logic below. However, consider importingjoinfrompathas well if you adoptpath.joinfor cleaner path construction (see comment on lines 50-52).
5-9: LGTM - cleaner API with callback pattern.The extended signature with
baseDirand optionalonDatacallback is a good design that provides more flexibility than the previous meta map approach. The default empty string forbaseDirmaintains reasonable backward compatibility for callers who don't need a root directory name.
15-17: Initialization correctly sets up directory tracking.The initialization of
current,stack, anddirStackwithbaseDiras the root is correct and consistent with the new API design.
22-22: Regex correctly extended to capture optional data segment.The addition of
(.*)?$as a fourth capture group enables extracting inline file data from the test DSL, which is then passed to theonDatacallback. The optional quantifier ensures backward compatibility with lines that don't include data.
28-29: Extraction logic correctly handles name and data.Trimming both
nameanddataappropriately removes surrounding whitespace, and the optional chaining safely handles cases where these capture groups are undefined.
37-37: Correct synchronization of dirStack with stack depth.When navigating up the directory tree (depth decreases), truncating
dirStackto matchstackensures the directory path array remains consistent with the current tree position.
47-47: Directory path correctly extended when entering subdirectories.Pushing the directory
nameontodirStackafter updatingstackproperly maintains the current path as you descend into subdirectories.packages/run/src/__tests__/fixtures/node-adapter-express-bodyParser/vite.config.ts (1)
7-9: Disabling HMR improves test determinism.The addition of
server: { hmr: false }is appropriate for test fixtures, as it prevents Hot Module Replacement from interfering with test execution and ensures consistent, reproducible behavior. This configuration has been applied consistently across all node-adapter test fixtures.packages/explorer/package.json (1)
18-19: LGTM! TypeScript types condition correctly positioned first.Placing the
"types"condition before"import"follows best practices and ensures TypeScript tooling can correctly resolve type definitions. This is a non-functional ordering improvement.examples/node-express/package.json (1)
22-22: No action required. TypeScript version 5.9.3 is actually newer than 5.7.2 (released October 1, 2025 vs. November 22, 2024), making this an upgrade rather than a downgrade. The version numbering correctly follows semantic versioning where higher version numbers indicate newer releases.Likely an incorrect or invalid review comment.
rturnq
force-pushed
the
route-entry-marko-api
branch
from
14a332a to
0687d97
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
packages/run/package.json (1)
36-36: Verify that@marko/vite@^5.4.2is published on npm.A previous review flagged that version 5.4.2 was not available on npm (latest was 5.3.0 at the time). Confirm this version has been published or correct the version specifier.
#!/bin/bash # Check the latest published version of @marko/vite on npm npm view @marko/vite versions --json | jq '.[-5:]' npm view @marko/vite versionpackages/run/src/__tests__/fixtures/force-class-api/transform.ts (1)
4-9: Guard against undefinedfilenameto prevent potential TypeError.The regex test will throw if
program.hub.file.opts.filenameis undefined. This was flagged in a previous review.Program(program: t.NodePath<t.Program>) { + const filename = program.hub.file.opts.filename ?? ""; if ( /dist[\\/]\.marko-run[\\/].+(?<!entry)\.marko$/.test( - program.hub.file.opts.filename, + filename, ) ) {packages/run/src/__tests__/fixtures/force-tags-api/transform.ts (1)
4-9: Guard against undefinedfilenameto prevent potential TypeError.This was flagged in a previous review. The regex test will throw if
program.hub.file.opts.filenameis undefined.Program(program: t.NodePath<t.Program>) { + const filename = program.hub.file.opts.filename ?? ""; if ( /dist[\\/]\.marko-run[\\/].+(?<!entry)\.marko$/.test( - program.hub.file.opts.filename, + filename, ) ) {packages/run/src/vite/utils/log.ts (1)
185-187: Consider using direct byte-length calculation for better efficiency.This was raised in a previous review. Using
Buffer.byteLengthfor strings and.byteLengthforUint8Arrayavoids unnecessary Blob allocation:function byteSize(source: string | Uint8Array): number { - return new Blob([source as BlobPart]).size; + return typeof source === "string" + ? Buffer.byteLength(source) + : source.byteLength; }packages/run/src/vite/plugin.ts (1)
144-156: Guard against emptyroute.layoutsarray to prevent runtime crash.This was flagged in a previous review. When a route has no layouts (only a
+page.marko), accessingroute.layouts[0].filePathwill throw. The cache logic with.has()is correct now, but the layout access needs guarding.let routeMarkoApiCache: Map<Route, string | undefined> | undefined; async function getMarkoApiForRoute(context: PluginContext, route: Route) { routeMarkoApiCache ??= new Map(); if (!routeMarkoApiCache.has(route)) { + const topLayout = route.layouts[0]; const markoAPI = - route.templateFilePath && - ((await loadModule(context, normalizePath(route.layouts[0].filePath))) + route.templateFilePath && topLayout && + ((await loadModule(context, normalizePath(topLayout.filePath))) ?.meta?.markoAPI as string | undefined); routeMarkoApiCache.set(route, markoAPI); return markoAPI; } return routeMarkoApiCache.get(route); }
🧹 Nitpick comments (1)
packages/run/src/__tests__/fixtures/force-class-api/transform.ts (1)
1-40: Consider extracting shared transform logic to reduce duplication.This file is identical to
force-tags-api/transform.ts. Consider extracting the sharedProgramvisitor to a common module that both fixtures can import.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (11)
package-lock.jsonis excluded by!**/package-lock.jsonand included by**packages/run/src/__tests__/fixtures/force-class-api/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/force-class-api/__snapshots__/preview.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/force-tags-api/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/__tests__/fixtures/force-tags-api/__snapshots__/preview.expected.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-class-api/__snapshots__/force-class-api.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-class-api/__snapshots__/force-class-api.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-class-api/__snapshots__/force-class-api.expected.routetypes.d.tsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-tags-api/__snapshots__/force-tags-api.expected.router.jsis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-tags-api/__snapshots__/force-tags-api.expected.routes.mdis excluded by!**/__snapshots__/**and included by**packages/run/src/vite/__tests__/fixtures/force-tags-api/__snapshots__/force-tags-api.expected.routetypes.d.tsis excluded by!**/__snapshots__/**and included by**
📒 Files selected for processing (33)
.changeset/spicy-moles-stop.md(1 hunks).github/workflows/ci.yml(3 hunks)examples/netlify/package.json(1 hunks)examples/node-express/package.json(1 hunks)examples/static/package.json(1 hunks)examples/zero-config/package.json(1 hunks)package.json(1 hunks)packages/run/package.json(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/.marko-run/routes.d.ts(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/marko.json(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/src/routes/+layout.marko(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/src/routes/+page.marko(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/test.config.ts(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/transform.ts(1 hunks)packages/run/src/__tests__/fixtures/force-class-api/tsconfig.json(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/.marko-run/routes.d.ts(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/marko.json(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/src/routes/+layout.marko(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/src/routes/+page.marko(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/test.config.ts(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/transform.ts(1 hunks)packages/run/src/__tests__/fixtures/force-tags-api/tsconfig.json(1 hunks)packages/run/src/__tests__/fixtures/netlify-adapter-edge/netlify.toml(1 hunks)packages/run/src/__tests__/fixtures/netlify-adapter-edge/test.config.ts(1 hunks)packages/run/src/__tests__/fixtures/netlify-adapter-not-edge/test.config.ts(1 hunks)packages/run/src/__tests__/main.test.ts(5 hunks)packages/run/src/vite/__tests__/fixtures/force-class-api/routes.txt(1 hunks)packages/run/src/vite/__tests__/fixtures/force-tags-api/routes.txt(1 hunks)packages/run/src/vite/__tests__/main.test.ts(5 hunks)packages/run/src/vite/__tests__/utils/fakeFS.ts(2 hunks)packages/run/src/vite/codegen/index.ts(3 hunks)packages/run/src/vite/plugin.ts(6 hunks)packages/run/src/vite/utils/log.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/run/src/tests/fixtures/force-class-api/marko.json
🚧 Files skipped from review as they are similar to previous changes (23)
- packages/run/src/tests/fixtures/force-tags-api/test.config.ts
- packages/run/src/tests/fixtures/force-class-api/test.config.ts
- packages/run/src/tests/fixtures/force-class-api/src/routes/+layout.marko
- packages/run/src/tests/fixtures/force-tags-api/marko.json
- packages/run/src/tests/fixtures/force-class-api/tsconfig.json
- packages/run/src/vite/tests/fixtures/force-tags-api/routes.txt
- examples/static/package.json
- packages/run/src/vite/tests/utils/fakeFS.ts
- packages/run/src/tests/fixtures/force-tags-api/tsconfig.json
- packages/run/src/tests/fixtures/netlify-adapter-not-edge/test.config.ts
- packages/run/src/vite/tests/fixtures/force-class-api/routes.txt
- packages/run/src/tests/fixtures/netlify-adapter-edge/test.config.ts
- packages/run/src/tests/fixtures/netlify-adapter-edge/netlify.toml
- packages/run/src/tests/fixtures/force-tags-api/src/routes/+layout.marko
- examples/node-express/package.json
- package.json
- packages/run/src/tests/fixtures/force-class-api/.marko-run/routes.d.ts
- packages/run/src/tests/main.test.ts
- examples/zero-config/package.json
- packages/run/src/tests/fixtures/force-tags-api/src/routes/+page.marko
- packages/run/src/tests/fixtures/force-class-api/src/routes/+page.marko
- examples/netlify/package.json
- packages/run/src/vite/tests/main.test.ts
🧰 Additional context used
🧬 Code graph analysis (5)
packages/run/src/__tests__/fixtures/force-class-api/transform.ts (1)
packages/run/src/__tests__/fixtures/force-tags-api/transform.ts (1)
Program(4-39)
packages/run/src/vite/codegen/index.ts (2)
packages/run/src/vite/codegen/writer.ts (1)
createStringWriter(165-179)packages/run/src/__tests__/fixtures/runtime-include/customAdapter.ts (1)
runtimeInclude(12-14)
packages/run/src/__tests__/fixtures/force-tags-api/transform.ts (1)
packages/run/src/__tests__/fixtures/force-class-api/transform.ts (1)
Program(4-39)
packages/run/src/vite/plugin.ts (3)
packages/run/src/vite/types.ts (1)
Route(87-97)packages/run/src/vite/utils/fs.ts (1)
normalizePath(5-8)packages/run/src/vite/codegen/index.ts (1)
renderRouteTemplate(37-63)
packages/run/src/__tests__/fixtures/force-tags-api/.marko-run/routes.d.ts (3)
packages/run/src/__tests__/fixtures/force-class-api/.marko-run/routes.d.ts (9)
Route(21-21)Route(33-33)Context(22-22)Context(34-34)Handler(23-23)Handler(35-35)route(25-25)route(37-37)Input(30-30)packages/run/src/vite/__tests__/fixtures/force-class-api/__snapshots__/force-class-api.expected.routetypes.d.ts (9)
Route(21-21)Route(33-33)Context(22-22)Context(34-34)Handler(23-23)Handler(35-35)route(25-25)route(37-37)Input(30-30)packages/run/src/vite/__tests__/fixtures/force-tags-api/__snapshots__/force-tags-api.expected.routetypes.d.ts (9)
Route(21-21)Route(33-33)Context(22-22)Context(34-34)Handler(23-23)Handler(35-35)route(25-25)route(37-37)Input(30-30)
🔇 Additional comments (13)
.changeset/spicy-moles-stop.md (1)
1-5: LGTM!The changeset correctly documents the patch release with a clear description that aligns with the PR objectives.
packages/run/src/vite/plugin.ts (5)
124-131: LGTM!Clean helper function that abstracts environment-aware module loading. The conditional logic correctly handles both the
transformRequestenvironment path and the fallbackcontext.loadpath.
597-599: LGTM!Good addition of
routeMarkoApiCache = undefinedto invalidate the cache when relevant file changes occur. This ensures fresh API resolution after layout file modifications.
300-304: LGTM!Correctly integrates per-route Marko API resolution into template rendering. The
awaitensures the API is resolved before rendering.
319-322: LGTM!Consistent with the normal routes handling—special routes also receive their resolved Marko API for template rendering.
694-695: LGTM!The non-null assertion is safe here since
virtualFiles.has(id)is checked on the line above.packages/run/src/vite/codegen/index.ts (3)
37-63: LGTM!The signature change from
(route, rootDir)to(route, markoApi?)aligns with the PR objective. The implementation correctly:
- Emits the
<!-- use ${markoApi} -->comment when the API is provided- Uses the writer pattern consistently with the rest of the codebase
- Properly constructs the entry template with nested layouts and page
277-284: LGTM!The parameter change from
runtimeInclude: string | undefinedtoruntimeInclude?: stringis semantically equivalent but more idiomatic TypeScript.
792-794: LGTM!Direct use of
route.path.pathsimplifies the code by removing the now-unnecessarydecodePathhelper call.packages/run/src/__tests__/fixtures/force-tags-api/.marko-run/routes.d.ts (1)
1-39: LGTM! Test fixture typings are correctly structured.The TypeScript module augmentations follow the correct pattern for Marko routing type definitions. The structure is consistent with the
force-class-apifixture, which aligns with the PR objective of testing that different Marko APIs produce compatible type outputs..github/workflows/ci.yml (3)
37-37: Good addition of Node 24 to the test matrix.Expanding test coverage to include Node 24 ensures compatibility with the latest Node.js version.
60-63: Appropriate permissions for release automation.The explicit permissions declaration is a security best practice. These permissions enable:
id-token: writefor npm provenance (package signing with OpenID Connect)contents: writefor creating releases and tagspull-requests: writefor the changesets bot to manage version PRs
66-70: Consistent action and Node version upgrades across jobs.The release job now uses the same action versions (v6) and Node version (24) as the build job, maintaining consistency across the CI pipeline.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #179 +/- ##
===========================
===========================
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
No description provided.