Repository navigation
[js] Remove automatic global polyfill - #1548
Conversation
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ 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". |
|
@bgentry This seem okay to you? |
bgentry
left a comment
bgentry
left a comment
There was a problem hiding this comment.
Implementation looks right to me: the check runs ahead of the modules that use Temporal at load, and I confirmed --import / NODE_OPTIONS preloads reach worker threads on Node 24.
One thing I'd change before merging: the CLI. On Node 24, npx riverqueue --help now dies at import with an uncaught ConfigurationError, because bin.ts statically imports run.js → @riverqueue/migrate → riverqueue. The "don't patch globals" argument is for the library; the CLI is an application and could own the polyfill itself (its own dependency, installed in bin.ts before a dynamic import of run.js). At minimum, catch the failure there and print a one-liner rather than a stack trace.
Nits: "Temporary" → "Temporal" in the description (twice); the scheduledAt minus one second change in the pg integration test looks like an unrelated flake fix worth a mention; the error message could link to #temporal-on-nodejs-24 directly.
|
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ 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". |
For JavaScript, remove automatic global polyfilling. River no longer installs or replaces `globalThis.Temporal`. `@js-temporary/polyfill` becomes a development dependency only. Missing Temporary provides a helpful `ConfigurationError` before modules evaluate and may find it missing. This produces the most helpful possible error message to help users along.
21a2cca to
1f2017a
Compare
🤖 For JavaScript, remove automatic global polyfilling from library imports. River no longer
installs or replaces
globalThis.Temporal. The core package keeps@js-temporal/polyfillas a development dependency only.When Temporal is missing, imports fail with a
ConfigurationErrorbeforemodules access it. The error points to the runtime setup instructions.
The Postgres integration fixture now schedules immediately claimable jobs one
second in the past. This fixes a pre-existing Node 24 test flake reproduced on
unchanged master: the polyfill's clock can run fractionally ahead of Postgres.