Use temporal-polyfill in package builds#925
Conversation
dahlia
added
runtime/node
✅ Deploy Preview for fedify-json-schema canceled.
|
|
@codex review |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughMigrates ChangesTemporal polyfill migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant mise
participant check_temporal_polyfill_ts
participant dist_outputs
participant tsc
mise->>check_temporal_polyfill_ts: run test:temporal-polyfill
check_temporal_polyfill_ts->>dist_outputs: scan package dist files
check_temporal_polyfill_ts->>tsc: run temporary consumer project checks
tsc-->>check_temporal_polyfill_ts: compile result
check_temporal_polyfill_ts-->>mise: exit status
Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: lockfile failed supply-chain policy check. Run 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 |
![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 replaces the runtime dependency @js-temporal/polyfill with temporal-polyfill across several packages, bundling it for CommonJS builds to ensure they are self-contained. A regression check script and shared build helpers are introduced to verify this packaging. The code review feedback highlights a cross-platform compatibility issue on Windows when executing tsc in the test script, a potential false positive in the declaration file check, and a quote-sensitivity issue in the import detection regex. Additionally, the reviewer noted that the skipNodeModulesBundle option used across multiple tsdown configurations is deprecated and redundant, and suggested marking @fedify/fixture as external in the testing package configuration.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Codex Review: Didn't find any major issues. Can't wait for the next one! 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". |
Codecov Report✅ All modified and coverable lines are covered by tests. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/debugger/tsdown.config.ts`:
- Around line 16-30: Remove the redundant skipNodeModulesBundle setting from the
cjs.deps block in tsdown.config.ts, since it conflicts with deps.alwaysBundle in
the same configuration. Keep the existing neverBundle and alwaysBundle logic
intact in the cjs configuration, and rely on the default behavior instead of
explicitly setting skipNodeModulesBundle to false.
In `@packages/mysql/tsdown.config.ts`:
- Around line 17-24: The CJS build config in the tsdown setup is using mutually
exclusive dependency options in the same deps block. Update the cjs section to
keep deps.alwaysBundle for the temporal polyfill logic and remove
deps.skipNodeModulesBundle entirely, then apply the same cleanup in the postgres
and redis tsdown.config files so validation passes consistently.
In `@packages/relay/tsdown.config.ts`:
- Around line 20-27: In the tsdown CJS configuration, remove the redundant
deps.skipNodeModulesBundle setting from the cjs block and keep only
deps.alwaysBundle with the temporal polyfill condition. Update the cjs object in
the tsdown.config setup so the deps options for temporalPolyfillImportPlugin and
temporalPolyfillCjsBanner no longer include mutually exclusive flags.
In `@packages/sqlite/tsdown.config.ts`:
- Around line 19-31: The sqlite tsdown cjs dependency config is using two
mutually exclusive bundle settings together. Update the cjs branch in
tsdown.config.ts to keep only deps.alwaysBundle with
isTemporalPolyfillDependency and remove deps.skipNodeModulesBundle, matching the
pattern used by the other package configs.
In `@scripts/tsdown/temporal.mts`:
- Around line 57-59: The temporalPolyfillIntro helper is currently a bare import
string with no format awareness, unlike temporalPolyfillEsmBanner and
temporalPolyfillCjsBanner. Update the temporal.ts helper so the intro is either
generated conditionally by output format or clearly restricted to ESM-only use,
and ensure any caller such as outputOptions.intro in tsdown.config.ts does not
inject it into CommonJS output. Preserve the existing format-specific behavior
used by the banner helpers when locating the change.
🪄 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: 39ec7e17-69ef-47c6-840e-3571eef5ffb5
⛔ Files ignored due to path filters (2)
deno.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (33)
CHANGES.mddeno.jsondocs/package.jsondocs/tutorial/astro-blog.mddocs/tutorial/content-sharing.mddocs/tutorial/microblog.mdmise.tomlpackages/cli/package.jsonpackages/cli/tsdown.config.tspackages/debugger/package.jsonpackages/debugger/tsdown.config.tspackages/fedify/package.jsonpackages/fedify/src/vocab/cjs.test.tspackages/fedify/tsdown.config.tspackages/mysql/package.jsonpackages/mysql/tsdown.config.tspackages/postgres/package.jsonpackages/postgres/tsdown.config.tspackages/redis/package.jsonpackages/redis/tsdown.config.tspackages/relay/package.jsonpackages/relay/tsdown.config.tspackages/sqlite/package.jsonpackages/sqlite/tsdown.config.tspackages/testing/package.jsonpackages/testing/tsdown.config.tspackages/vocab/deno.jsonpackages/vocab/package.jsonpackages/vocab/src/cjs.test.tspackages/vocab/tsdown.config.tspnpm-workspace.yamlscripts/check_temporal_polyfill.tsscripts/tsdown/temporal.mts
Switch Temporal-backed packages to temporal-polyfill and bundle it into CommonJS outputs so consumers no longer require @js-temporal/polyfill or a global Temporal at runtime. Add regression coverage for the CommonJS entries and a packaging check that keeps CJS artifacts self-contained while declarations use the standard esnext.temporal library reference. The packaging check also compiles sample TypeScript consumers with Bundler and NodeNext module resolution. fedify-dev#823 fedify-dev#925 Assisted-by: Codex:gpt-5.5
dahlia
force-pushed
the
issue-823-temporal-polyfill
branch
from
878c10e to
bf5132d
Compare
Remove redundant skipNodeModulesBundle settings from the Temporal CJS build configs while keeping the intended alwaysBundle behavior. Centralize the nested deps shape in the shared helper so tsdown's current config types do not require the deprecated option at call sites. Make the packaging check more portable by running TypeScript through node and by locating the compiler entry in the workspace package store. Also avoid requiring Temporal lib references from declaration files that do not mention Temporal, and make the injected import guard handle single-quoted imports. fedify-dev#925 (comment) fedify-dev#925 (comment) fedify-dev#925 (comment) fedify-dev#925 (comment) fedify-dev#925 (comment) fedify-dev#925 (comment) fedify-dev#925 (comment) fedify-dev#925 (comment) fedify-dev#925 (comment) fedify-dev#925 (comment) fedify-dev#925 (comment) fedify-dev#925 (comment) fedify-dev#925 (comment) fedify-dev#925 (comment) fedify-dev#925 (comment) fedify-dev#925 (comment) fedify-dev#925 (comment) Assisted-by: Codex:gpt-5.5
|
/gemini review |
|
@codex review |
There was a problem hiding this comment.
Code Review
This pull request replaces the @js-temporal/polyfill dependency with temporal-polyfill across all packages and updates the documentation and tutorials. It introduces shared build helpers to bundle temporal-polyfill in CommonJS outputs to ensure they are self-contained, and adds a regression check script to verify packaging. The review feedback suggests improving the regression check script by using a more precise regular expression to detect Temporal references in declaration files and dynamically selecting the symlink type to ensure cross-platform compatibility on Windows.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@deno.json`:
- Around line 47-48: The imports map entry for temporal-polyfill is in the wrong
section; keep `@js-temporal/polyfill` in the scoped block, and move
temporal-polyfill into the unscoped block near tsdown so the key ordering
matches the existing grouping in deno.json. Update the imports map entries in
the same area to preserve the scoped-first, unscoped-later structure.
In `@packages/vocab/src/cjs.test.ts`:
- Around line 11-19: The test in cjs.test.ts uses rejects() with
Promise.resolve().then(...) even though require("temporal-polyfill") throws
synchronously, so simplify this assertion by switching to assert.throws()
instead. Update the assertion import to use throws from node:assert, and keep
the same error-code check logic while removing the unnecessary promise wrapper
around require("temporal-polyfill").
In `@pnpm-workspace.yaml`:
- Around line 61-62: The catalog entry for temporal-polyfill is out of
alphabetical order in the workspace dependencies block. Reposition
temporal-polyfill in pnpm-workspace.yaml so the catalog keeps the existing
convention used elsewhere: `@-scoped` entries first, then unscoped entries sorted
alphabetically, placing temporal-polyfill near tsdown/typescript rather than
immediately after `@js-temporal/polyfill`.
In `@scripts/check_temporal_polyfill.ts`:
- Around line 155-162: The consumer type-check setup in Deno.symlink currently
relies on the earlier build loop having already produced each package’s dist
output, because the symlinks point at packages/${pkg} source trees and
resolution depends on package.json exports/types. Make this dependency explicit
in scripts/check_temporal_polyfill.ts by either guarding the symlink phase with
a clear check that dist exists or adding a brief note near the
typeConsumerPackages loop and Deno.symlink block so future maintainers
understand the ordering requirement.
🪄 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: 2966030e-d50a-4655-89cd-c2bcad0fa9da
⛔ Files ignored due to path filters (2)
deno.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (33)
CHANGES.mddeno.jsondocs/package.jsondocs/tutorial/astro-blog.mddocs/tutorial/content-sharing.mddocs/tutorial/microblog.mdmise.tomlpackages/cli/package.jsonpackages/cli/tsdown.config.tspackages/debugger/package.jsonpackages/debugger/tsdown.config.tspackages/fedify/package.jsonpackages/fedify/src/vocab/cjs.test.tspackages/fedify/tsdown.config.tspackages/mysql/package.jsonpackages/mysql/tsdown.config.tspackages/postgres/package.jsonpackages/postgres/tsdown.config.tspackages/redis/package.jsonpackages/redis/tsdown.config.tspackages/relay/package.jsonpackages/relay/tsdown.config.tspackages/sqlite/package.jsonpackages/sqlite/tsdown.config.tspackages/testing/package.jsonpackages/testing/tsdown.config.tspackages/vocab/deno.jsonpackages/vocab/package.jsonpackages/vocab/src/cjs.test.tspackages/vocab/tsdown.config.tspnpm-workspace.yamlscripts/check_temporal_polyfill.tsscripts/tsdown/temporal.mts
|
Codex Review: Didn't find any major issues. 🚀 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". |
Use a word-boundary Temporal declaration check, make the temporary package links friendlier to Windows, and document the dist output dependency in the consumer type-check setup. Also keep the new temporal-polyfill entries in the existing dependency order and simplify the CommonJS regression test's synchronous require assertion. fedify-dev#925 (comment) fedify-dev#925 (comment) fedify-dev#925 (comment) fedify-dev#925 (comment) fedify-dev#925 (comment) fedify-dev#925 (comment) Assisted-by: Codex:gpt-5.5
|
/gemini review |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! 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". |
1 participant
Summary
This switches the Temporal-backed package builds from @js-temporal/polyfill to temporal-polyfill, keeps ESM artifacts importing temporal-polyfill directly, bundles the same polyfill into CommonJS artifacts, and adds regression coverage for the package formats affected by issue #823.
How it works
The package dependencies now use temporal-polyfill consistently across the Temporal-backed packages. The Deno import map in deno.json and the relevant package manifests under packages/ were updated together so Deno, Node.js, and Bun resolve the same intended polyfill family.
The build configuration change is centralized in scripts/tsdown/temporal.mts. Each affected tsdown.config.ts imports that helper instead of duplicating the same dependency handling in every package. The helper makes ESM output import temporal-polyfill directly, because ESM consumers can resolve the package through its supported export surface. For CommonJS output, it marks temporal-polyfill for bundling so generated CJS artifacts do not emit
require("temporal-polyfill"), which fails because the package does not expose a CommonJS require entry.The public declaration files continue to describe the standard Temporal API rather than the implementation package. That means generated declarations use the
esnext.temporallibrary reference and do not expose @js-temporal/polyfill, temporal-polyfill, or temporal-spec as user-facing type dependencies. This keeps the public API aligned with standardTemporaltypes and avoids making consumers depend on an implementation-specific module name in their type surface.The regression checks are split between runtime entry checks and packaging checks. packages/vocab/src/cjs.test.ts and packages/fedify/src/vocab/cjs.test.ts cover CommonJS loading behavior for the package entries that previously depended on Temporal at runtime. scripts/check_temporal_polyfill.ts scans built artifacts for forbidden CommonJS
require()output, verifies declaration files keep the standard Temporal library reference, and compiles sample TypeScript consumers with bothmoduleResolution: "Bundler"andmoduleResolution: "NodeNext".The test:temporal-polyfill task in mise.toml now runs the packaging check after the build. It grants read, write, and run permissions because the check creates temporary consumer projects under tmp/, symlinks local packages into their node_modules/@fedify/ trees, and invokes the workspace TypeScript compiler against those projects.
Why this approach
The main compatibility issue is not just the package rename. temporal-polyfill should be the implementation used by both ESM and CommonJS package builds, but the two module formats need different packaging treatment. ESM can keep a direct
importfrom temporal-polyfill, while CommonJS cannot emit a barerequire("temporal-polyfill")in current Node.js. Bundling only the CJS side addresses that specific incompatibility while preserving the cleaner direct import behavior for ESM builds.Centralizing the build behavior in scripts/tsdown/temporal.mts keeps the package configs consistent. The same rule applies to @fedify/vocab, @fedify/fedify, the database adapters, @fedify/testing, @fedify/relay, @fedify/debugger, and @fedify/cli, so putting the policy in one helper makes future build changes less error-prone and makes the reason for the CommonJS exception visible in one place.
Keeping declarations implementation-agnostic is important because users should type their code against the standard Temporal API, not against whichever package Fedify currently uses internally to provide it. That also leaves room for native Temporal support in runtimes without forcing a public API change later.
The additional TypeScript consumer checks cover the part of issue #823 that artifact scanning alone cannot prove. It is possible for generated files to avoid forbidden imports while still failing in modern application templates because of package exports, declaration resolution, or module resolution mode differences. Compiling temporary projects with Bundler and NodeNext resolution exercises the declarations the same way downstream TypeScript users are likely to consume them.