fix(core): preserve new.target in the deterministic Date override so Date subclasses work - #3372
fix(core): preserve new.target in the deterministic Date override so Date subclasses work#3372ar-tama wants to merge 5 commits into
new.target in the deterministic Date override so Date subclasses work#3372Conversation
🦋 Changeset detectedLatest commit: 4c7cd3c The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 packages
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 |
|
@ar-tama is attempting to deploy a commit to the Vercel Labs Team on Vercel. A member of the Team first needs to authorize it. |
TooTallNate
left a comment
There was a problem hiding this comment.
Reviewed at 922287c (base is current main, 0 behind). Build green; all 37 vm tests pass; full core suite green except the one pre-existing failure you correctly identified (more on that below).
Verified:
- The final
Reflect.construct(Date_, args, new.target)design is the right call over the class approach from the PR description: a plain function keeps[[Call]], so bareDate()stays callable — and now returns the spec-correct string instead of the old non-spec object. Construction semantics all check out: a plain function retains[[Construct]],super()from a subclass forwardsnew.targetthrough toDate_, and the constructed object (with the subclass's prototype and a real Date internal slot) becomesthis— identity, methods, and fields preserved, which is exactly whatTZDateneeds. - Determinism is fully preserved: zero-arg construction and zero-arg
super()both pin tofixedTimestamp,Date.nowstays overridden (and is inherited by subclass statics via the retainedsetPrototypeOf), andfixedTimestampis a liveletbinding thatupdateTimestampadvances during replay — the closure reads the current value at each construction, same as before. - Cross-engine alignment bonus: the QuickJS engine (
WORKFLOW_VM=quickjs) never wrapsDateat all — it intercepts the WASIclock_time_getsyscall, so itsDateis fully native and spec-correct. That means QuickJS never had this subclassing bug, and your bare-Date()string change brings node:vm into agreement with QuickJS where the old object-return diverged. No QuickJS-side work needed. - One honest behavioral note for the record: a workflow that called bare
Date()and used the old non-spec object (e.g..getTime()on it) would diverge when an in-flight run is replayed across this upgrade. That pattern is vanishingly rare, was already broken outside the sandbox, and the new behavior is what both the spec and the other engine do — the right trade.
Two asks before merge:
- DCO check is failing — the commits need sign-off (rebase with
--signoff, or follow the DCO bot's remediation instructions). - The changeset text is stale: it describes the override as "now a class that preserves
new.target", but the final implementation is a plain function usingReflect.construct(your head commit changed the approach to keep callability). Changesets become release notes — please reword, and consider mentioning that bareDate()now returns the spec time string.
On the unrelated runtime.test.ts failure: confirmed — re-routes a misrouted lazy hook resume with its payload intact fails on an unmodified checkout of main. It's an interaction between two recently merged PRs (#2960's deployment-affinity guard and #3345's lazy-hook fast path); I've reported it with analysis on #3345. Thank you for isolating it precisely instead of ignoring it — that note made the triage immediate.
Clean, well-reasoned fix with exactly the right tests (subclass identity + zero-arg determinism, bare-call string, static inheritance). Approving.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: ar_tama <arata.makoto@gmail.com>
… so `Date` subclasses work in workflow functions The VM's `Date` override was a plain function, so `class X extends Date` lost the subclass identity: `super()` returned a fresh plain `Date` that became `this`, dropping the subclass's methods and fields. This silently broke `Date` subclasses like `TZDate` from `@date-fns/tz`. Using `class Date extends Date_` keeps `new.target` intact, and `extends` already wires up the prototype chain and statics, so the manual `prototype` assignment and `Object.setPrototypeOf` fix-ups are no longer needed. Determinism is unchanged: zero-arg construction still returns the fixed timestamp and `Date.now()` is still overridden. Fixes vercel#3371 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: ar_tama <arata.makoto@gmail.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: ar_tama <arata.makoto@gmail.com>
Use a plain function that branches on `new.target` and constructs via `Reflect.construct(Date_, args, new.target)` instead of a class: subclassing still works (`new.target` is forwarded), and calling `Date()` without `new` now matches the spec — arguments are ignored and the (fixed) time string is returned, where the previous override returned a `Date` object. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: ar_tama <arata.makoto@gmail.com>
…entation Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: ar_tama <arata.makoto@gmail.com>
922287c to
4c7cd3c
Compare
|
Both asks addressed: rebased with |
Fixes #3371
Problem
createContext()replaces the VM's globalDatewith a plain function. A plain function has no[[Construct]]behavior that forwardsnew.target, so when user code doesclass X extends Date,super()returns a fresh plainDateobject which becomesthis— the subclass instance loses its identity, methods, and fields.This silently breaks every library that models a date by subclassing
Date, e.g.TZDatefrom@date-fns/tz(the officially recommended way to do time-zone-aware math with date-fns v4). Inside a workflow body, aTZDatedegrades to a plainDate, so every zone-aware getter falls back to the container's time zone. Since the epoch value stays correct, this presents as a silent, production-only off-by-one-day bug. Full analysis in #3371.Fix
Replace the plain-function override with a class so
new.targetis preserved:Both of the previous fix-ups become unnecessary, because
extendsalready sets up the whole prototype chain:(g as any).Date.prototype = Date_.prototype— withextendsthe real chain is in place (and the assignment would be illegal on a class, whoseprototypeis non-writable).Object.setPrototypeOf(g.Date, Date_)—extendsalready makes the statics (Date.parse,Date.UTC) inherited; only theDate.nowoverride stays as an own property.Determinism is unchanged: zero-arg construction still returns the fixed timestamp, and
Date.now()is still overridden. Covered by the existing determinism tests plus two new ones (subclassing, statics).One behavioral note: calling
Date()withoutnewnow throws (classes are not callable), where the old override returned aDateobject — itself already a deviation from the spec, which returns a string. If callableDate()needs to keep working, the alternative is a plain function that branches onnew.targetand usesReflect.construct(Date_, args, new.target); happy to switch to that if preferred.Tests
should support subclassing \Date`— subclass keeps identity (instanceof`), methods, fields; constructor args are forwarded; zero-arg subclass construction still gets the fixed timestampshould preserve \Date` static methods—Date.parse/Date.UTC` still inheritedAll 36 tests in
packages/core/src/vm/index.test.tspass,pnpm typecheckis clean. (Theruntime.test.tsfailurere-routes a misrouted lazy hook resumealso fails on an unmodified checkout ofmain, so it is unrelated.)🤖 Generated with Claude Code