[srikrsna]

No description provided.

[changeset-bot]

🦋 Changeset detected

Latest commit: 6cbd973

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
capnweb	Minor

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

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/cloudflare/capnweb@278

npm i https://pkg.pr.new/cloudflare/capnweb/capnweb-validate@278

commit: 6cbd973

[teamchong]

/bonk review this

[ask-bonk]

The encoding isn't lossless for Temporal.Duration. toJSON() folds sub-second units into the seconds field, so the round trip changes the duration's fields even though the total stays the same. Checked with Node --harmony-temporal:

D.from({milliseconds: 1500}).toJSON()          // "PT1.5S"
D.from("PT1.5S")                               // seconds: 1, milliseconds: 500 (was milliseconds: 1500)
D.from({seconds: 59, milliseconds: 1000})      // -> "PT60S" -> seconds: 60, milliseconds: 0

That's a spec limitation of Duration's ISO string form, so the wire format can't avoid it without encoding the fields one by one. Please narrow the claim here (and in the comment at src/serialize.ts:394), for example: "Durations keep their total length, but sub-second units may be rebalanced into seconds." The alternative is a field-wise encoding if callers need exact fields.

[ask-bonk]

Review: 1 finding.

I posted 1 inline comment on the PR.

Ran Node with --harmony-temporal to check: a Temporal.Duration doesn't round-trip exactly. Converting it to a string merges sub-second units into the seconds field, so {milliseconds: 1500} comes back as {seconds: 1, milliseconds: 500}. The total length is the same but the individual fields aren't. The protocol docs call the encoding "lossless", and so does the comment at src/serialize.ts:394. That wording should be narrowed. The other option is to send durations field by field.

Not raised inline:

• packages/capnweb-validate (type-introspector.ts BUILTIN_VALUE_TYPES) doesn't know about the new Temporal types yet. Validated interfaces that use them will likely be reported as unsupported. This could be done in a follow-up.

github run

[teamchong]

clanky review

[clanky-the-clanker]

💬 Approved with comments — The Temporal serialization is implemented cleanly and is tested. There is one WARNING: the validator package and its docs do not cover the new Temporal types. There is also one INFO about the Temporal tests possibly skipping silently in Node.

Findings: 1 warning · 1 info

_{The decision is advisory: this comment never approves, blocks or requests changes on the pull request.}

🔧 Fix with your agent

Paste the following into your coding agent to investigate and fix these issues.

Fix the following code review issues found in pull request #278.

Before making changes, review each issue and present a brief summary table:
- For each finding, state whether you agree, disagree, or need clarification
- If you disagree (e.g., the fix would require disproportionate changes
  for minimal ROI, or the finding is factually incorrect), explain why
- If you need to clarify something before deciding, ask those questions
- Then share your plan for which issues to tackle and in what order

## WARNING (1)

### [Documentation] validation.mdx says every RPC-supported type has a build-time validator, but capnweb-validate has no Temporal support (type-introspector.ts:523, emit.ts:221). Users of the validator may see Temporal-typed methods rejected or left unchecked. Add support or document the gap.
- **File:** packages/docs/src/content/docs/guides/validation.mdx:192

## INFO (1)

### [Code Quality] If the --harmony-temporal execArgv in vitest.config.ts does not reach the `node` project, the Temporal tests skip silently because of describe.skipIf(!Temporal). Consider a guard or setting execArgv per project.
- **File:** vitest.config.ts:13

---

📋 Code Quality: 💬 1 info

The implementation is correct and covers every typeForRpc consumer in core.ts. There is one INFO about the tests possibly skipping silently.

Recommendation: 💬 Optional Nits

• [INFO] If the --harmony-temporal execArgv in vitest.config.ts does not reach the node project, the Temporal tests skip silently because of describe.skipIf(!Temporal). Consider a guard or setting execArgv per project. (vitest.config.ts:13)

View full review

Silent skip risk (vitest.config.ts:13-17, __tests__/index.test.ts:3357-3360). The --harmony-temporal flag is set through root-level poolOptions.forks.execArgv, and the suite is wrapped in describe.skipIf(!Temporal).

If the node project never inherits that flag, the whole Temporal suite skips in Node CI and the run still passes. This review did not run vitest, so whether the root-level option reaches the node project is unconfirmed. Two possible fixes:

• Add a guard that fails when Node is 24 or newer and Temporal is missing.
• Set execArgv on the node project directly.

Checked and found fine:

• Where the Temporal check sits in typeForRpc.
• Every TypeForRpc switch in src/core.ts: deepCopy treats the types as immutable; dispose and followPath are no-ops.
• Tuple encoding in Devaluator.
• Evaluator checks tuple length and string type before calling from().
• The error path when the runtime has no Temporal.
• TemporalType collapses to never when no Temporal lib is present.

---

📝 Documentation: ⚠️ 1 warning

The validator docs and the validator package do not account for the newly supported Temporal types.

Recommendation: 📝 Minor Comments

• [WARNING] validation.mdx says every RPC-supported type has a build-time validator, but capnweb-validate has no Temporal support (type-introspector.ts:523, emit.ts:221). Users of the validator may see Temporal-typed methods rejected or left unchecked. Add support or document the gap. (packages/docs/src/content/docs/guides/validation.mdx:192)

View full review

TemporalType is now part of the RPC-compatible types (src/types.d.ts). However, packages/docs/src/content/docs/guides/validation.mdx:192-196 still says that every type Cap'n Web guarantees can travel over RPC also has a precise build-time validator, and its list of built-ins leaves out Temporal.

capnweb-validate also has no Temporal entry:

• The built-in table in packages/capnweb-validate/src/transform/type-introspector.ts:523 maps URL and similar types but not Temporal.
• The emit switch in packages/capnweb-validate/src/transform/emit.ts:221 has a url case but no Temporal case.

So users who rely on the validator may hit build-time rejections, or get no runtime checks, for methods that take Temporal values. Either add validator support or document the limitation in validation.mdx and packages/capnweb-validate/README.md.

---

✅ All other checks passed

🚀 Release: ✅ LGTM

Risk: Low. The changeset is present and fits an additive feature.

---

🔒 Security: ✅ LGTM

No security issues found. The Evaluator checks the shape of incoming Temporal tuples before calling Temporal.*.from().

---

📖 AGENTS.md: ✅ LGTM

No issues found.

---

⚡ Performance: ✅ LGTM

No performance issues found.

---

_{Reviewed commit: 6cbd973e}