fix: improve interop translator #240
Conversation
🦋 Changeset detectedLatest commit: d4e4f61 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 two changeset files declaring patch releases. Updates GitHub Actions usage to v6, Node versions to 24, permission blocks for test/release jobs, and Playwright step adjustments. Bumps multiple dependencies and devDependencies. Refactors Tags API detection: Pre-merge checks and finishing touches❌ Failed checks (1 warning, 2 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (14)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
🧰 Additional context used🧬 Code graph analysis (2)src/__tests__/fixtures/isomorphic-tags-api-basic/dev-server.mjs (3)
src/__tests__/fixtures/isomorphic-tags-api-basic/server.mjs (2)
⏰ 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). (3)
🔇 Additional comments (12)
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: 1
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
63-63: Consider upgrading actions/cache and codecov-action for consistency.
actions/cache@v4andcodecov/codecov-action@v3were not upgraded while other actions moved to v6. This may be intentional, but verify these are the latest stable versions.Also applies to: 76-76
📜 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 (5)
.changeset/fast-words-drop.md(1 hunks).github/workflows/ci.yml(3 hunks)package.json(1 hunks)src/index.ts(5 hunks)src/server-entry-template.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/server-entry-template.ts (1)
src/render-assets-runtime.ts (1)
renderAssetsRuntimeId(1-1)
🔇 Additional comments (10)
.changeset/fast-words-drop.md (1)
1-5: LGTM!The changeset correctly declares a patch release with a concise description that aligns with the PR objectives.
.github/workflows/ci.yml (3)
81-84: Permissions setup looks correct for npm provenance publishing.The
id-token: writepermission combined with the removal ofNPM_TOKENsuggests migration to OIDC-based npm publishing with provenance. This is a good security practice. Ensure the npm publish command in your release script includes the--provenanceflag if this is the intended approach.
47-47: Node.js 24 is available inactions/setup-nodeand is currently in Active LTS status (as of October 2025). No action needed; the matrix configuration with versions 20, 22, and 24 is valid and supported on GitHub runners.
20-26: > Likely an incorrect or invalid review comment.src/server-entry-template.ts (2)
14-29: LGTM!The changes improve clarity by:
- Using static
<Template ...input/>instead of dynamic invocation, which aligns with the import statement- Adding HTML comments (
<!-- use tags -->) to identify the code path for debugging purposes
32-49: LGTM!Consistent application of the same improvements (static Template reference, HTML comment marker) to the class API code path.
package.json (1)
43-52: Dependency updates look reasonable.The runtime dependency updates (@chialab/cjs-to-esm, resolve) are minor version bumps that should be backward compatible.
src/index.ts (3)
174-204: Well-structured refactoring of isTagsApi detection.The change from a lazy boolean to a parameterized function enables per-entry API detection, which is the core improvement for interop translator handling. The caching of
defaultIsTagsAPIensures backward compatibility when no explicit API is provided.
974-981: Consistent metadata propagation.The meta payload now includes
markoAPIfrom the compiler'smeta.api, which enables the per-entry API detection used in server entry template generation.
942-949: Correct usage of per-file API detection for hot-update logic.The hot-update injection now correctly uses
isTagsApi(meta.api)instead of the global default, aligning with the interop improvements.
DylanPiercey
force-pushed
the
interop-improvements
branch
from
a6ff83b to
bf2967b
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #240 +/- ##
==========================================
- Coverage 72.38% 72.10% -0.28%
==========================================
Files 12 12
Lines 793 839 +46
Branches 213 229 +16
==========================================
+ Hits 574 605 +31
- Misses 177 192 +15
Partials 42 42 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
DylanPiercey
force-pushed
the
interop-improvements
branch
from
bf2967b to
7e1df1a
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
.github/workflows/ci.yml (2)
58-60: Use--with-depsonly on Linux runners to avoid unnecessary overhead on Windows/macOS.The
--with-depsflag installs OS-level dependencies, which is only relevant on Linux; on Windows and macOS it's redundant and can cause unnecessary delays. Condition the installation step:- - name: Playwright Install - run: npx playwright install --with-deps chromium + - name: Playwright Install (Linux deps) + if: runner.os == 'Linux' + run: npx playwright install --with-deps chromium + - name: Playwright Install (non-Linux) + if: runner.os != 'Linux' + run: npx playwright install chromium
67-71: Removeid-token: writeunless Trusted Publishing is configured on npmjs.com.npm provenance is only auto-generated when using npm Trusted Publishing (OIDC), which requires explicit configuration on the npm package. If this package doesn't use Trusted Publishing,
id-token: writeadds unnecessary permissions and can be safely removed. Thecontents: writeandpull-requests: writepermissions are correct for changesets/action.src/__tests__/fixtures/isomorphic-tags-api-basic/src/tags/stateless.marko (1)
1-5: Potential null reference in browser initialization.
document.body.firstElementChildcould benullif the body has no child elements, which would causeappend()to throw. Since this is a test fixture with controlled DOM structure, this is acceptable, but consider adding a guard if reused elsewhere.static { if (typeof window === "object") { - document.body.firstElementChild.append("Loaded stateless tag"); + document.body.firstElementChild?.append("Loaded stateless tag"); } }src/server-entry-template.ts (1)
13-29: Ensureopts.entryDataelements are safe literals before string interpolationBecause
[${opts.entryData.join(",")}]is injected into generated source, any non-literal/unescaped content could break generation. IfentryDataisn’t guaranteed to already be serialized (e.g., viaJSON.stringify), consider serializing here.src/__tests__/fixtures/isomorphic-tags-api-basic/server.mjs (1)
12-16: Useres.writableEnded(or add try/catch) for a more robust fallback decisionIf
handlerends the response without sending headers in an edge case, the static fallback could attempt to write again. Consider:export default createServer(async (req, res) => { - await handler(req, res); - if (res.headersSent) return; - await serve(req, res, serveOpts); + try { + await handler(req, res); + if (res.headersSent || res.writableEnded) return; + await serve(req, res, serveOpts); + } catch (err) { + res.statusCode = 500; + res.end("Internal Server Error"); + } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
package-lock.jsonis excluded by!**/package-lock.jsonand included by**src/__tests__/fixtures/isomorphic-tags-api-basic/__snapshots__/build.expected.mdis excluded by!**/__snapshots__/**and included by**src/__tests__/fixtures/isomorphic-tags-api-basic/__snapshots__/dev.expected.mdis excluded by!**/__snapshots__/**and included by**
📒 Files selected for processing (13)
.changeset/fast-words-drop.md(1 hunks).github/workflows/ci.yml(3 hunks)package.json(1 hunks)src/__tests__/fixtures/isomorphic-tags-api-basic/dev-server.mjs(1 hunks)src/__tests__/fixtures/isomorphic-tags-api-basic/server.mjs(1 hunks)src/__tests__/fixtures/isomorphic-tags-api-basic/src/index.js(1 hunks)src/__tests__/fixtures/isomorphic-tags-api-basic/src/tags/layout.marko(1 hunks)src/__tests__/fixtures/isomorphic-tags-api-basic/src/tags/stateful.marko(1 hunks)src/__tests__/fixtures/isomorphic-tags-api-basic/src/tags/stateless.marko(1 hunks)src/__tests__/fixtures/isomorphic-tags-api-basic/src/template.marko(1 hunks)src/__tests__/fixtures/isomorphic-tags-api-basic/test.config.ts(1 hunks)src/index.ts(8 hunks)src/server-entry-template.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- .changeset/fast-words-drop.md
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🧬 Code graph analysis (3)
src/__tests__/fixtures/isomorphic-tags-api-basic/dev-server.mjs (2)
src/__tests__/fixtures/isomorphic-tags-api-basic/server.mjs (1)
__dirname(9-9)src/__tests__/fixtures/isomorphic-tags-api-basic/src/index.js (1)
handler(3-9)
src/__tests__/fixtures/isomorphic-tags-api-basic/server.mjs (2)
src/__tests__/fixtures/isomorphic-tags-api-basic/dev-server.mjs (1)
__dirname(12-12)src/__tests__/fixtures/isomorphic-tags-api-basic/src/index.js (1)
handler(3-9)
src/server-entry-template.ts (1)
src/render-assets-runtime.ts (1)
renderAssetsRuntimeId(1-1)
⏰ 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). (3)
- GitHub Check: test: node@24 (windows-latest)
- GitHub Check: test: node@22 (windows-latest)
- GitHub Check: test: node@20 (windows-latest)
🔇 Additional comments (14)
.github/workflows/ci.yml (1)
24-27: The configuration is supported.actions/setup-node@v6officially supports Node 24, and the workflow correctly usescache: npmwhich is the required explicit cache input for v6. There are no Windows-specific breaking changes in v6; the matrix setup with ubuntu-latest and windows-latest works as expected. Cache keys are platform-aware (expected behavior, not an issue), and the workflow's explicit cache declaration ensures proper caching across all OS runners.src/index.ts (6)
174-204: Well-structured parameterized API detection.The refactor cleanly separates explicit per-entry API hints from the fallback translator-based detection. The caching of
defaultIsTagsAPIensures the translator lookup only happens once.Note: The catch block at line 197 now defaults to
true(tags API) when the translator cannot be loaded, whereas before it may have had different behavior. Verify this is the intended fallback.
786-806: LGTM!The
cachedSourcesfallback additions at lines 788 and 806 provide consistent source resolution across virtual and regular file loads, supporting the per-entry metadata propagation.
863-870: Verify module metadata availability on first load.The
this.load({ id: fileName })call retrieves the module'smeta.markoAPIvalue. If this is invoked before the module has been transformed (first access),loaded.meta.markoAPImay beundefined, causingisTagsApi(undefined)to fall back to translator detection.This is likely acceptable since the fallback behavior is the previous default, but verify this ordering is intentional and that the fallback produces correct results for all entry scenarios.
918-923: LGTM!The meta object now consistently includes
markoAPIalongside existing arc-related fields, enabling per-entry API detection in downstream processing.
946-953: LGTM!Correctly passes
meta.apifrom the compiled output toisTagsApi(), ensuring hot-reload behavior aligns with each template's actual API type rather than a global default.
975-985: LGTM!The transform return correctly propagates
markoAPIin both build and dev modes, while keeping arc-related metadata build-only.src/__tests__/fixtures/isomorphic-tags-api-basic/src/template.marko (1)
1-9: LGTM!Clean test fixture demonstrating isomorphic tags API usage with layout composition and custom tag inclusion.
src/__tests__/fixtures/isomorphic-tags-api-basic/src/tags/stateless.marko (1)
7-9: LGTM!Valid tag composition structure for testing nested stateful components within a stateless wrapper.
src/__tests__/fixtures/isomorphic-tags-api-basic/src/tags/stateful.marko (1)
1-7: LGTM!Well-crafted test fixture demonstrating isomorphic state management with
<let/>tags, client-side hydration (via themountedflag), and interactive event handling. This effectively validates the tags API flow.src/__tests__/fixtures/isomorphic-tags-api-basic/src/tags/layout.marko (1)
7-15: Layout structure/content projection looks correct for the fixture
<${input.content}/>in<body>is the right shape for a layout tag used as a wrapper in fixtures.src/__tests__/fixtures/isomorphic-tags-api-basic/dev-server.mjs (1)
9-11: VerifycreateRequire(... )("../../..").defaultworks in this fixture layoutThis relative require is fragile; if the fixture directory depth changes, the Marko plugin import breaks. Consider resolving from a known package name (if possible) or add a brief comment indicating the expected fixture placement.
src/server-entry-template.ts (1)
14-29: Template marker +<Template ...input/>swap looks consistent across branchesThe two branches now emit clearly distinguishable output and consistently use
<Template ...input/>, which should reduce downstream ambiguity.Also applies to: 32-49
src/__tests__/fixtures/isomorphic-tags-api-basic/test.config.ts (1)
1-4: Thepageglobal is reliably injected by the test harness and is properly typed as global; no changes needed.The test harness in
src/__tests__/main.test.tsinitializesglobalThis.pagein itsbefore()hook (line 123) and declares it as a global type (lines 18–29). All test steps execute after this hook completes, sopageis always available. The code is correct as written.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/index.ts (1)
174-204: Good direction: per-entry Tags API detection + cached default fallback.
Only nit: consider typing the arg asapi?: string(same semantics, cleaner surface).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/ripe-baths-fold.md(1 hunks)src/index.ts(8 hunks)
⏰ 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). (3)
- GitHub Check: test: node@24 (windows-latest)
- GitHub Check: test: node@20 (windows-latest)
- GitHub Check: test: node@22 (windows-latest)
🔇 Additional comments (6)
.changeset/ripe-baths-fold.md (1)
1-5: Changeset looks correct and appropriately scoped to a patch bump.src/index.ts (5)
786-807:load()fallback tocachedSourcesseems correct for linked/dev flows.
946-953: HMR guard now depends on per-file API (meta.api) — seems aligned with the goal.
Just verifymeta.apiis always set (or that default fallback behavior is intended when it’s missing).
966-976: Re-check the optional watch list for Tags API entries.
Formeta.api === "tags", you now only watch${optionalFilePrefix}style.*. If Tags API entries can still be affected by adjacentcomponent.*/component-browser.*conventions in your ecosystem, this may regress reload behavior.
978-983: Includingmeta.analyzedTagsin watch files is a nice improvement for indirect dependencies.
918-923: Remove unusedarcSourceCodeandarcScanIdsmeta fields—they are dead code and never consumed.These fields are assigned in the transform hook return but never read by any code in the codebase. They do not propagate to client-side manifests (which are generated from HTML parsing via
generateDocManifest), do not cause memory bloat, and do not expose source code to the client. ThebrowserManifestis built independently and doesn't include these fields.Since
arcSourceId(line 811) demonstrates that module metadata is actually read when needed, the absence of any reads ofarcSourceCodeorarcScanIdsconfirms these are unused and should be removed to keep the code clean.Also applies to: 988-995
Likely an incorrect or invalid review comment.
DylanPiercey
force-pushed
the
interop-improvements
branch
from
3507276 to
d4e4f61
Compare
No description provided.