Accept Temporal values from any spec-conformant implementation#768
Accept Temporal values from any spec-conformant implementation#768dahlia wants to merge 16 commits into
Temporal values from any spec-conformant implementation#768Conversation
TypeScript 6.0 ships `lib.esnext.temporal` with the ambient `Temporal` namespace, which lets generated `.d.ts` files reference `Temporal.Instant`/`Temporal.Duration` without importing the polyfill module. This is a prerequisite for resolving the type-level conflict between the polyfill's nominal classes and native `Temporal` values described in fedify-dev#767. Assisted-by: Claude Code:claude-opus-4-7
The Activity Vocabulary constructors and `clone()` methods rejected `Temporal.Instant`/`Temporal.Duration` values produced by any implementation other than the bundled `@js-temporal/polyfill` — for example, the native `Temporal` shipped with Node.js 26+ — throwing `TypeError: The published must be of type Temporal.Instant.` even though the value was spec-conformant. The runtime check used `instanceof` against the polyfill's class, which never matches a foreign class instance. Introduce `isTemporalInstant()` and `isTemporalDuration()` helpers in `@fedify/vocab-runtime` that use `Symbol.toStringTag` — mandated by the Temporal specification — so any spec-conformant value passes, regardless of which class produced it. Route the codegen and the one direct `instanceof` site in `@fedify/fedify` through the helpers, and add a regression test that exercises the foreign-instance path. Resolves fedify-dev#767. Assisted-by: Claude Code:claude-opus-4-7
The bundled runtime polyfill is still useful for environments without
native `Temporal`, but emitting `import { Temporal } from
"@js-temporal/polyfill"` into the consumer-facing `.d.ts` binds the
declared types to the polyfill's nominal classes. Users on Node.js
26+ pass native `Temporal.Instant` values and TypeScript flags them
as the wrong type even though they are structurally identical.
Split tsdown's `outputOptions.intro` into `banner.js` (keeps the
polyfill import / require for runtimes that lack native `Temporal`)
and `banner.dts` (emits `/// <reference lib="esnext.temporal" />` so
the ambient `Temporal` namespace from `lib.esnext.temporal` resolves
the generated type references). Both polyfill and native values now
satisfy the declared types.
Refs fedify-dev#767.
Assisted-by: Claude Code:claude-opus-4-7
Refs fedify-dev#767. Assisted-by: Claude Code:claude-opus-4-7
`Symbol.toStringTag` alone is sufficient to identify spec-conformant
`Temporal` values, but it leaves bare objects whose tag was set
without exposing the rest of the type's shape (e.g. `{ [Symbol
.toStringTag]: "Temporal.Instant" }`) passing as Temporal values.
Those would fail later, at serialization time, with a less obvious
error.
Check `epochNanoseconds` for `Temporal.Instant` and `sign` for
`Temporal.Duration` — accessors that the spec mandates and that
distinguish the type's identity — so misuse fails fast at the
constructor boundary instead of leaking into later code paths.
Assisted-by: Claude Code:claude-opus-4-7
Mirror the `@fedify/vocab-runtime/jsonld` precedent and surface the new guards under `@fedify/vocab-runtime/temporal` only. Consumers that need just the type guards no longer have to pull the rest of the runtime — docloader, multibase, key handling, and so on — through the import graph, which matters for CommonJS bundles where tree-shaking is limited. Route the codegen import and the one in-tree consumer (`packages/fedify/src/utils/kv-cache.ts`) through the subpath so the workspace itself follows the public surface. Refs fedify-dev#767. Assisted-by: Claude Code:claude-opus-4-7
`@fedify/postgres`, `@fedify/redis`, and `@fedify/sqlite` accept `Temporal.Duration`/`Temporal.DurationLike` in their public message- queue options. Their `.d.ts` files imported `Temporal` from `@js-temporal/polyfill`, binding the declared parameter types to the polyfill's nominal classes — so users on Node.js 26+ passing native `Temporal.Duration` values hit the same nominal mismatch as with `@fedify/fedify`. Apply the same `banner.js`/`banner.dts` split that `@fedify/vocab` and `@fedify/fedify` use, so the runtime polyfill import stays only in the JS output while the type declarations resolve `Temporal` through `lib.esnext.temporal`. Convert `@fedify/relay`, `@fedify/testing`, and `@fedify/debugger` as well — these packages don't expose `Temporal` in their public API, but their `.d.ts` inherits the dead polyfill import from the old intro, and keeping the configuration consistent makes future audits easier. Refs fedify-dev#767. Assisted-by: Claude Code:claude-opus-4-7
The Deno, Node, and Bun snapshots of `generateClasses()` still contained the old `instanceof Temporal.*` checks and lacked the `@fedify/vocab-runtime/temporal` import. Regenerate them so the class.test.ts suite matches the current codegen output. Refs fedify-dev#767. Assisted-by: Claude Code:claude-opus-4-7 Assisted-by: Codex:gpt-5.5
dahlia
added
type/bug
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
dahlia
changed the title
Temporal values from any spec-conformant implementation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request resolves a TypeError triggered when Activity Vocabulary constructors receive Temporal objects from native host implementations—such as Node.js 26+—rather than the bundled polyfill. The fix involves replacing instanceof checks with new Symbol.toStringTag-based type guards in @fedify/vocab-runtime/temporal and updating build configurations to reference the esnext.temporal library. Feedback highlights that the TypeScript version bump to ^6.0.0 might be premature and suggests using Object.prototype.toString.call() in the type guards for better cross-realm robustness.
JSR rejects published packages that contain triple-slash directives which modify globals, and `/// <reference lib="esnext.temporal" />` in _packages/vocab-runtime/src/temporal.ts_ tripped the `banned-triple-slash-directives` check during `deno publish --dry-run` in the release-test CI job. The dist banner already injects the same directive into the built _*.d.ts_ files for npm consumers, so the source-level directive was duplicated work. Drop it from the source. Deno's TypeScript 6.0 loads `lib.esnext.temporal` in its default lib, so the source still type-checks under `deno check`, and tsdown picks up the ambient `Temporal` namespace the same way during dts emission. Refs fedify-dev#767. Assisted-by: Claude Code:claude-opus-4-7
|
@codex review |
Codecov Report✅ All modified and coverable lines are covered by tests.
🚀 New features to boost your workflow:
|
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Both `isTemporalInstant()` and `isTemporalDuration()` now read the
spec-mandated `Symbol.toStringTag` through
`Object.prototype.toString.call(value)`. MDN documents this as the
canonical way to identify built-in objects that expose a custom tag,
and dropping the `{ [Symbol.toStringTag]?: unknown }` cast leaves
the guard with two short, readable conditions. The secondary
`epochNanoseconds`/`sign` checks remain so spoofed tags on bare
objects are still rejected.
fedify-dev#768 (comment)
fedify-dev#768 (comment)
Assisted-by: Claude Code:claude-opus-4-7
|
/gemini review |
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Code Review
This pull request resolves a TypeError caused by nominal type mismatches between different Temporal implementations—such as native Node.js 26+ versus the bundled polyfill—by replacing instanceof checks with Symbol.toStringTag-based type guards. New utility functions isTemporalInstant() and isTemporalDuration() are introduced in @fedify/vocab-runtime, and code generation tools are updated to utilize these guards. Additionally, the project's TypeScript version is upgraded to 6.0, and build configurations are adjusted to reference the ambient Temporal namespace. Feedback suggests further hardening the new type guards by explicitly verifying the types of identifying properties like epochNanoseconds and sign.
The TS 6 catalog bump in this PR exposed three issues in the VitePress + Twoslash code-block pipeline. `@typescript/vfs` injects a `baseUrl` when it spins up the virtual TypeScript environment for Twoslash, and TS 6 emits TS5101 for any `baseUrl` use (the option is gone in TS 7). Setting `ignoreDeprecations: "6.0"` on the Twoslash compiler options silences the diagnostic. Several Twoslash blocks did `import "@types/bun"` to load Bun globals. TS 6 rejects importing `@types/...` declaration files directly with TS6137 and points at the canonical module name; the fix is to switch to `import "bun"` in federation.md, relay.md, and the basics tutorial. A handful of Twoslash blocks in pragmatics.md, vocab.md, and the microblog tutorial imported `Temporal` from `@js-temporal/polyfill` and then constructed Fedify vocab objects with the result. After the earlier banner change in this PR, Fedify's generated `.d.ts` resolves `Temporal.Instant`/`Temporal.Duration` through `lib.esnext.temporal`, so a polyfill-typed `Temporal.Instant` no longer matches the constructor parameter (nominal mismatch on `until(...).sign`). Drop the polyfill imports; the ambient `Temporal` namespace from Twoslash's `esnext` lib supplies the right types. One demo block in microblog.md that purposely shows the polyfill import line is demoted from `twoslash` to plain `typescript` so it keeps its place in the prose without being type-checked. The surrounding narrative still recommends installing the polyfill for Node.js < 26, which remains correct at runtime. https://github.com/fedify-dev/fedify/actions/runs/25843983520/job/75935220881 Assisted-by: Claude Code:claude-opus-4-7
`isTemporalInstant()` and `isTemporalDuration()` now follow the `Symbol.toStringTag` check and the `"X" in value` presence check with a `typeof` check on the identifying property: `epochNanoseconds` must be a `bigint` for `Temporal.Instant`, `sign` must be a `number` for `Temporal.Duration`. The spec mandates both types, so this catches the bare-object case where a caller spoofs the tag and adds a property of the wrong shape (for example, `epochNanoseconds: 0` instead of `0n`). Keeping the `in` check before the `typeof` access lets TypeScript narrow `value` to the right shape, so no `as any` cast is needed. Two regression tests cover the new branches: a `Temporal.Instant`- tagged object with a number-valued `epochNanoseconds`, and a `Temporal.Duration`-tagged object with a string-valued `sign`. fedify-dev#768 (comment) fedify-dev#768 (comment) Assisted-by: Claude Code:claude-opus-4-7
|
/gemini review |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e6852a9ba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Code Review
This pull request upgrades the project to TypeScript 6.0 and refactors the handling of Temporal values to support native implementations alongside the existing polyfill. Key changes include the introduction of Symbol.toStringTag-based type guards in @fedify/vocab-runtime, updating the vocabulary toolchain to use these guards instead of instanceof checks, and refactoring build configurations to use the banner property in tsdown. I have no feedback to provide.
The rule normalizer in `kv-cache.ts` used to be `d instanceof Temporal.Duration ? d : Temporal.Duration.from(d)`, so anything that wasn't a real polyfill `Duration` got pushed through `from()` and validated. Earlier in this PR the `instanceof` branch was replaced with `isTemporalDuration(d)`, but that guard intentionally accepts any spec-conformant `Duration`, including bare objects whose `Symbol.toStringTag` is spoofed and that carry a numeric `sign`. Such an object would flow through as a TTL into stores like `MemoryKvStore.set()`, whose `options.ttl.round(...)` then throws at write time. Codex flagged this as a P1 regression in input validation. Drop the conditional and always call `Temporal.Duration.from(d)`. The spec defines `Duration.from()` to accept either a real `Duration` or a `DurationLike` object, validate the shape, and return a fresh `Duration`. Real instances pass through cheaply; malformed objects fail at the rule-matching boundary instead of leaking into the cache. The now-unused `isTemporalDuration` import goes too. fedify-dev#768 (comment) Assisted-by: Claude Code:claude-opus-4-7
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request upgrades the project to TypeScript 6.0 and addresses compatibility issues with native Temporal implementations by replacing nominal instanceof checks with Symbol.toStringTag-based type guards in @fedify/vocab-runtime. Code generation and build configurations were updated to utilize these new guards and include necessary ambient type references via banners. Feedback suggests verifying if explicit triple-slash references are required in source files to prevent IDE errors and recommends conditionally importing the new type guards in generated classes to avoid potential unused-import warnings.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15f0d34412
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The previous guard surface (`Symbol.toStringTag` plus the
`epochNanoseconds`/`sign` presence and type check) accepted
shape-only spoofs like
`{ [Symbol.toStringTag]: "Temporal.Instant", epochNanoseconds: 0n }`.
A vocab constructor would store such an object, and the generated
`toJsonLd()` later does `value.toString()`. When `toString` is
inherited from `Object.prototype`, that call returns
`"[object Temporal.Instant]"` and we emit a non-RFC 3339 timestamp
into JSON-LD. Codex flagged this as a P2 regression.
Real polyfill and host-native Temporal classes always override
`toString` to emit ISO 8601 output, so checking
`value.toString !== Object.prototype.toString` cleanly separates
them from spoofs. Add that check, with the `"toString" in value &&
typeof value.toString === "function"` guards in front so TypeScript
narrows `value` and the `!==` comparison type-checks without a
cast. This stays in the spirit of the earlier "one distinctive
property" decision: one more spec-mandated invariant (Temporal
types override `toString`), not arbitrary methods.
Tests:
- The "accepts spec-compliant non-polyfill objects" fixtures and
the "rejects non-bigint/non-number" fixtures now define a
`toString` so they isolate the property-type checks.
- New cases exercise the new branch directly: a plain object whose
`toString` is the inherited default, for both Instant and
Duration.
- The vocab regression tests for "accepts foreign Temporal.Instant
(issue fedify-dev#767)" mock a `toString` too, so the foreign mock is a
closer stand-in for a real native `Temporal.Instant`.
fedify-dev#768 (comment)
Assisted-by: Claude Code:claude-opus-4-7
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request resolves a TypeError encountered when using native Temporal implementations (e.g., Node.js 26+) by replacing nominal instanceof checks with structural Symbol.toStringTag-based type guards in a new @fedify/vocab-runtime/temporal subpath. The workspace has been upgraded to TypeScript 6.0, and generated type declarations now utilize the esnext.temporal library to ensure compatibility across different Temporal implementations. Build configurations and documentation examples were also updated to support these changes and suppress TypeScript 6.0 deprecation warnings. I have no feedback to provide as there were no review comments to assess.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd023a2b72
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Per the Temporal spec, `Temporal.Duration.prototype.sign` returns one
of `-1`, `0`, or `1`. The earlier guard only required `typeof
value.sign === "number"`, so an object like
`{ [Symbol.toStringTag]: "Temporal.Duration", sign: 42, ... }` passes
and reaches the JSON-LD serializer, where the spurious value shows up
as an `xsd:duration` that the constructor input never implied. Codex
flagged this as a P2 regression versus the pre-PR `instanceof`
branch.
Replace `typeof value.sign === "number"` with
`value.sign === -1 || value.sign === 0 || value.sign === 1`.
TypeScript narrows `value.sign` to `number` after the `in` check, so
the strict-equality chain type-checks without a cast. Real polyfill
and host-native `Temporal.Duration` instances expose one of these
three values, so they continue to pass. Round-tripping through
`Temporal.Duration.from()` was considered but rejected because it
would widen the input contract to also accept ISO 8601 strings.
A new test covers the new branch: a spec-shaped mock whose `sign`
is `42` is rejected. The existing "rejects non-number sign" case
(with `sign: "0"`) still passes; a string is not strict-equal to any
of the three allowed numbers.
fedify-dev#768 (comment)
Assisted-by: Claude Code:claude-opus-4-7
|
@codex review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request fixes a TypeError caused by nominal type mismatches between native Temporal implementations (e.g., Node.js 26+) and the bundled polyfill. The solution introduces Symbol.toStringTag-based type guards in a new @fedify/vocab-runtime/temporal module and updates code generation to use these guards instead of instanceof. The project is upgraded to TypeScript 6.0, allowing type declarations to reference the ambient Temporal namespace via the esnext.temporal library. Build configurations were refactored to include necessary triple-slash references in generated declarations, and documentation was updated to reflect the removal of explicit polyfill imports in many contexts. I have no feedback to provide as there were no review comments.
|
Codex Review: Didn't find any major issues. Swish! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/vocab-runtime/src/temporal.test.ts`:
- Around line 1-4: The test imports `test` from "node:test" which violates the
guideline to use runtime-agnostic fixtures; replace the import so that `test` is
imported from "@fedify/fixture" instead of "node:test" in this file
(temporal.test.ts) and update any related import bindings (e.g., the existing
`test` import) so the tests continue to call `test(...)` as before; ensure other
imports (`isTemporalDuration`, `isTemporalInstant`, `Temporal`, `strictEqual`)
remain unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f7096f46-27ec-4185-b959-f2535038833a
⛔ Files ignored due to path filters (5)
deno.lockis excluded by!**/*.lockpackages/vocab-tools/src/__snapshots__/class.test.ts.deno.snapis excluded by!**/*.snappackages/vocab-tools/src/__snapshots__/class.test.ts.node.snapis excluded by!**/*.snappackages/vocab-tools/src/__snapshots__/class.test.ts.snapis excluded by!**/*.snappnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (26)
CHANGES.mddocs/.vitepress/config.mtsdocs/manual/federation.mddocs/manual/pragmatics.mddocs/manual/relay.mddocs/manual/vocab.mddocs/tutorial/basics.mddocs/tutorial/microblog.mdpackages/debugger/tsdown.config.tspackages/fedify/src/utils/kv-cache.tspackages/fedify/tsdown.config.tspackages/postgres/tsdown.config.tspackages/redis/tsdown.config.tspackages/relay/tsdown.config.tspackages/sqlite/tsdown.config.tspackages/testing/tsdown.config.tspackages/vocab-runtime/deno.jsonpackages/vocab-runtime/package.jsonpackages/vocab-runtime/src/temporal.test.tspackages/vocab-runtime/src/temporal.tspackages/vocab-runtime/tsdown.config.tspackages/vocab-tools/src/class.tspackages/vocab-tools/src/type.tspackages/vocab/src/vocab.test.tspackages/vocab/tsdown.config.tspnpm-workspace.yaml
💤 Files with no reviewable changes (2)
- docs/manual/pragmatics.md
- docs/manual/vocab.md
1 participant
Closes #767.
Summary
Activity Vocabulary constructors and
clone()methods rejectedTemporal.Instant/Temporal.Durationvalues produced by anything other than the bundled@js-temporal/polyfill. On Node.js 26+, that meant the nativeTemporalthrewTypeError: The published must be of type Temporal.Instant., and TypeScript flagged native instances as the wrong type even when the runtime would have accepted them. The generated declarations had the same problem in@fedify/postgres,@fedify/redis, and@fedify/sqlite, wherepollIntervalandhandlerTimeoutcarried the polyfill's nominal classes through the generated .d.ts.Runtime validation now uses a
Symbol.toStringTagguard and also checksepochNanosecondsonInstantorsignonDuration, so objects with only a spoofed tag are still rejected. Generated .d.ts files no longer importTemporalfrom@js-temporal/polyfill; they reference the ambientTemporalnamespace throughlib.esnext.temporalinstead, so both polyfill and native values satisfy the declared types. This requires TypeScript 6.0, the first version that shipslib.esnext.temporal.The guards are exported from
@fedify/vocab-runtime/temporal, following the existing@fedify/vocab-runtime/jsonldsubpath, so callers can import the guard without importing the whole runtime. The JS output still imports the polyfill at runtime, preserving Node 22-25 support.Breaking change
Consumers must use TypeScript 6.0 or later. The generated .d.ts references
lib.esnext.temporal, which TS 5.x doesn't ship. CHANGES.md notes this under each affected package.Test plan
mise run checkpassesmise run test:denopasses (22,027Deno tests across vocab + actor + lookup + type suites)@fedify/vocab,@fedify/fedify,@fedify/postgres,@fedify/redis,@fedify/sqlite(cd packages/<name> && pnpm pretest && cd dist && node --test)dist/**/*.d.tsin those packages no longer imports@js-temporal/polyfilland starts with/// <reference lib="esnext.temporal" />dist/**/*.{js,cjs}in those packages still imports the polyfill (runtime fallback for Node ≤ 25)globalThis.Temporalpatched in) thatnew Article({ published: globalThis.Temporal.Instant.from("…") })no longer throws