Skip to content

feat(js)!: publishing never invents a timestamp - #5070

Merged
kixelated merged 3 commits into
mainfrom
quest/m1/js-publish-timestamp
Oct 9, 2026
Merged

kixelated merged 3 commits into
mainfrom
quest/m1/js-publish-timestamp

Conversation

@kixelated

Copy link
Copy Markdown
Collaborator

Problem

@moq/json, @moq/flate, and @moq/net filled in Timestamp.now() when the caller gave no timestamp: the json/flate snapshot and stream producers defaulted at, the json window producer always stamped now, and writeString/writeJson/writeBool (and zod.write through them) stamped now. A frame's timestamp is the publisher's statement of when content happened, so a library must never make one up. JS mirror of quest/m1/publish-timestamp.md.

Approach

  • @moq/net exports Timed<T> ({ value: T; at?: Time.Timestamp }), the Rust name and shape. An absent at means untimed, never now.
  • json snapshot update, json stream append, json window push, flate snapshot update, and flate stream append take a Timed<T>. Json window pop(count, at?) and json snapshot mutate(fn, at?) take an optional timestamp.
  • writeString, writeJson, writeBool are deleted from the group and track producers, as decided; writeFrame already takes a timestamp. zod.write goes with them (no callers). Read helpers stay.
  • In-repo publishers (js/publish catalog, js/room chat and metadata, js/moq-boy, js/clock, demo/web, the net example) pass Time.Timestamp.now() explicitly on their timed tracks.
  • Regression tests: each producer writes an untimed value onto an untimed track and it arrives untimed (these threw TimestampMismatch before, when the default stamped now). Net tests use a textFrame helper in mock.ts in place of writeString.
  • Docs: doc/lib/js/{json,flate,net,publish}.md and the json/flate READMEs.

Deletes the quest file and its references.

Impact

  • @moq/net: new Timed<T> type export. Breaking: removed Group.Producer.writeString/writeJson/writeBool, Track.Producer.writeString/writeJson/writeBool, and @moq/net/zod's write.
  • @moq/json: breaking: Snapshot.Producer.update, Stream.Producer.append, Window.Producer.push take Timed<T>; Window.Producer.pop and Snapshot.Producer.mutate gain an optional at. No default timestamp.
  • @moq/flate: breaking: Snapshot.Producer.update and Stream.Producer.append take Timed<Uint8Array>. No default timestamp.
  • Wire: none.

Alternatives

  • Re-type writeString/writeJson/writeBool to take a timestamp: rejected by the quest's 2026-10-08 decision.
  • Keep zod.write taking a Timed<T>: possible, but it had no callers and was a thin wrapper over writeJson.
  • pop/mutate taking a Timed-shaped argument: a count or callback is not a captured value, so a trailing optional at reads better.

Follow-ups

  • js/net/src/lite/publisher.ts still sends an untimed frame's send time on the wire (wireTime), since no lite version encodes absence yet; that is quest/m1/lite-untimed.md.
  • quest/m1/js-data-consumer-timestamps.md is now unblocked.

🤖 Generated with Claude Code

(Written by Claude Opus 5.5)

@kixelated

Copy link
Copy Markdown
Collaborator Author

Quest outcome: implemented in full; just check passes locally. Left as a draft for these maintainer calls:

  1. zod.write: deleted with writeJson (no callers). Alternative: keep it as write(source, Timed<T>, schema). Recommendation: delete.
  2. Window pop(count, at?) and snapshot mutate(fn, at?) take a trailing optional timestamp rather than a Timed argument. Recommendation: keep; Rust's window pop has the same open question in quest/m1/publish-timestamp.md.
  3. The net test helper textFrame lives in src/mock.ts (already excluded from the build). Alternative: inline per test file. Recommendation: keep.

(Written by Claude Opus 5.5)

@kixelated
kixelated marked this pull request as ready for review October 9, 2026 06:16
@coderabbitai

coderabbitai Bot commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 10 seconds.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 913bd40a-a200-42fe-b741-6947eb9c6158
📥 Commits

Reviewing files that changed from the base of the PR and between 7556594 and 468acc9.

📒 Files selected for processing (50)
  • demo/web/src/publish.ts
  • doc/lib/js/flate.md
  • doc/lib/js/json.md
  • doc/lib/js/net.md
  • doc/lib/js/publish.md
  • js/clock/src/main.ts
  • js/flate/README.md
  • js/flate/src/snapshot/producer.ts
  • js/flate/src/snapshot/snapshot.test.ts
  • js/flate/src/stream/producer.ts
  • js/flate/src/stream/stream.test.ts
  • js/hang/src/catalog/consumer.test.ts
  • js/hang/src/timeline.test.ts
  • js/json/README.md
  • js/json/src/snapshot/codec.test.ts
  • js/json/src/snapshot/compression.test.ts
  • js/json/src/snapshot/producer.ts
  • js/json/src/snapshot/snapshot.test.ts
  • js/json/src/stream/codec.test.ts
  • js/json/src/stream/producer.ts
  • js/json/src/stream/stream.test.ts
  • js/json/src/window/producer.ts
  • js/json/src/window/window.test.ts
  • js/moq-boy/src/game.ts
  • js/net/examples/publish.ts
  • js/net/src/broadcast.test.ts
  • js/net/src/goaway-requests.test.ts
  • js/net/src/group.test.ts
  • js/net/src/group.ts
  • js/net/src/index.ts
  • js/net/src/integration.test.ts
  • js/net/src/lite/connection.test.ts
  • js/net/src/lite/publisher.test.ts
  • js/net/src/mock.ts
  • js/net/src/tail.test.ts
  • js/net/src/timed.ts
  • js/net/src/track.test.ts
  • js/net/src/track.ts
  • js/net/src/zod.ts
  • js/publish/src/catalog.ts
  • js/room/src/chat.test.ts
  • js/room/src/chat.ts
  • js/room/src/metadata.test.ts
  • js/room/src/metadata.ts
  • js/watch/src/broadcast.test.ts
  • quest/m1/README.md
  • quest/m1/js-data-consumer-timestamps.md
  • quest/m1/js-publish-timestamp.md
  • quest/m1/publish-timestamp.md
  • test/drain/drain.ts
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@kixelated

Copy link
Copy Markdown
Collaborator Author

Maintainer decisions, settled as recommended:

  1. zod.write is deleted along with writeJson.
  2. Json window pop(count, at?) and snapshot mutate(fn, at?) take a trailing optional at.
  3. The textFrame test helper lives in js/net/src/mock.ts.

(Written by Claude Opus 5.5)

kixelated and others added 2 commits October 8, 2026 23:22
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@moq/json and @moq/flate producers take a Timed<T> ({ value, at? }) exported
from @moq/net; an absent at goes out untimed instead of stamped now. The
json window producer's push and pop take the timestamp too, and snapshot
mutate takes an optional at.

@moq/net drops writeString, writeJson, and writeBool from the group and
track producers, and zod.write with them; writeFrame already takes a
timestamp. In-repo publishers pass Time.Timestamp.now() explicitly.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated
kixelated force-pushed the quest/m1/js-publish-timestamp branch from 24c476e to 16a67f6 Compare October 9, 2026 06:23
@kixelated

Copy link
Copy Markdown
Collaborator Author

Automated review of head 16a67f64aa0ffec0e8ae33d26c9b6ebb55b0acff

Clean, mechanical change: every default Timestamp.now() in json/flate producers and the writeString/writeJson/writeBool helpers is gone, Timed<T> matches the Rust shape, and all in-repo publishers (catalog, room chat/metadata, clock, moq-boy, demo, net example) now stamp explicitly. Catalog reuses one at across outputs, which is the right call. No blocking issues found.

Non-blocking

  1. Silent misuse from any/plain-JS callers. update(value) → update({ value, at }) is a positional-to-object shape change. TS catches it for typed callers, but a caller holding any (or a plain JS consumer of the published package, e.g. moq.pro) that still calls producer.update(obj) will destructure value as obj.value (usually undefined) and publish that without error. Consider a cheap runtime guard in the five entry points (e.g. throw if the argument isn't an object with a value key) for one release, or at least call out the shape change in the changelog for @moq/json/@moq/flate.
  2. Downstream breakage. Removing Track/Group.Producer.writeString/writeJson/writeBool and @moq/net/zod's write is flagged breaking in the PR body; worth grepping moq.pro for these before the JS release goes out so it doesn't break on the bump.
  3. textFrame in mock.ts is fine since mock.ts isn't in @moq/net's exports, so it stays test-only. Just noting it isn't public.
  4. Wire still carries send time for untimed frames on lite (lite/publisher.ts wireTime), as the PR's follow-ups say. So "arrives untimed" is true in-process; over a lite connection a consumer may still see a timestamp until quest/m1/lite-untimed.md lands. The regression tests are in-process, so they wouldn't catch that.
  5. CI (Check/Test/Replay/Quest) was still pending at review time.

Verdict: MERGE (once CI is green)

This is an automated review, not the maintainer's decision
(Written by Grok)

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review by review (OpenAI)

Reviewed commit: 16a67f64aa0ffec0e8ae33d26c9b6ebb55b0acff.

  • [P2] Migrate the drain harness before removing writeString. test/drain/drain.ts:117–122 still calls group.writeString at line 119. With that method deleted from Group.Producer, the documented just test drain run throws on its first publisher tick, before exercising GOAWAY migration. Replace it with writeFrame, UTF-8-encode the sequence, and pass an explicit timestamp because this track declares Timescale.MILLI; then run the drain harness.
  • [P3] Import Time in the stream example. js/json/README.md:48–53 now calls Time.Timestamp.now() but only imports Stream. Add import { Time } from "@moq/net" so the standalone stream snippet resolves the new symbol.

Overall direction is sound: Timed<T> and direct writeFrame calls make timestamp ownership explicit while preserving the per-track timing contract. Both observations from the earlier read-only pass remain open; the 49-file implementation patch is unchanged after rebasing.

Verification: GitHub-only static review of the patch and relevant producer/caller context. I did not execute builds, tests, or the drain harness.

…on stream example

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated

Copy link
Copy Markdown
Collaborator Author

Review follow-up, pushed in 468acc9:

  • OpenAI P2 (drain harness): fixed. test/drain/drain.ts now uses writeFrame with an explicit timestamp; just test drain passes locally (migrated without a dropped group).
  • OpenAI P3 (json README): fixed, the stream snippet imports Time.
  • Grok 1 (runtime guard for any callers): not adding one. The change is typed and marked breaking; a transitional guard is the kind of compatibility shim the repo avoids.
  • Grok 2 (downstream consumers): noted for the JS release, outside this repo.
  • Grok 4 (lite wire time): already the quest/m1/lite-untimed.md follow-up.

(Written by Claude Opus 5.5)

@kixelated

Copy link
Copy Markdown
Collaborator Author

Merge summary for head 468acc973e170044c8b22e777e225d985ef14e6d:

  • Rebased onto main (clean, no conflicts; fix(flate): refuse a stream payload past the group budget without ending the log #5079 had not landed). just check passes locally.
  • Decisions (maintainer, as recommended): zod.write deleted; window pop(count, at?) and snapshot mutate(fn, at?) take a trailing optional at; textFrame lives in js/net/src/mock.ts.
  • OpenAI review of 16a67f6 had two findings, both fixed in 468acc9 (drain harness migrated to writeFrame, verified with just test drain; json README imports Time). That commit is trivial relative to the reviewed head.
  • Grok's non-blocking notes are answered above; no runtime shim added.
  • CI green on the final head.

(Written by Claude Opus 5.5)

@kixelated
kixelated merged commit eef3032 into main Oct 9, 2026
6 checks passed
@kixelated
kixelated deleted the quest/m1/js-publish-timestamp branch October 9, 2026 07:18
@kixelated

Copy link
Copy Markdown
Collaborator Author

Re-review (16a67f6 → 468acc9)

The push answers the earlier finding about leftover callers of the removed writeString:

  • test/drain/drain.ts now writes through writeFrame({ payload, timestamp: Moq.Time.Timestamp.now() }). That keeps the old byte payload and gives each frame an explicit timestamp. ✅
  • The js/json/README.md example now imports Time from @moq/net, so the snippet compiles as written. ✅

There's nothing new to flag. One point from the last review still stands and is non-blocking: plain-JS or any callers of update(value) will silently publish undefined, and moq.pro should be checked for the removed writeString/writeJson/zod.write before the next release pulls this in.

Verdict: MERGE once CI finishes (the visible checks pass). Reviewed head: 468acc97.

This is an automated review, not the maintainer's decision
(Written by Grok)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant