Skip to content

node: enable Temporal support#281707

Open
Leask wants to merge 1 commit into
Homebrew:mainfrom
Leask:codex/node-temporal-support
Open

node: enable Temporal support#281707
Leask wants to merge 1 commit into
Homebrew:mainfrom
Leask:codex/node-temporal-support

Conversation

@Leask
Copy link
Copy Markdown

@Leask Leask commented May 9, 2026

This replaces #281626 with the corrected approach for enabling Temporal in Node 26.

The original attempt only added Rust / Cargo and a Temporal test, but CI confirmed Node disables Temporal when built with system/shared ICU:

WARNING: Temporal support disabled when compiling with a shared ICU library

This revision avoids the shared/system ICU path for Node 26 by:

  • keeping rust as a build dependency;
  • using Node's default Temporal auto-enable path;
  • replacing system ICU with full ICU;
  • adding the Node-pinned ICU 78.3 source tarball as a Homebrew resource;
  • passing a local ICU source path to keep the build reproducible and avoid configure-time network downloads;
  • retaining a formula test that asserts Temporal.Now.zonedDateTimeISO exists.

Local verification:

brew style node
brew audit --strict node

I also ran a configure-stage check locally with the same Homebrew shared dependencies. The generated config.gypi contains:

"v8_enable_temporal_support": 1

Refs:

Copilot AI review requested due to automatic review settings May 9, 2026 02:00
@github-actions github-actions Bot added rust Rust use is a significant feature of the PR or issue long build Set a long timeout for formula testing long dependent tests Set a long timeout for dependent testing labels May 9, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Enables V8 Temporal support in the node formula (Node 26) by avoiding the shared/system ICU build path that disables Temporal, and adds a formula test to confirm Temporal is available.

Changes:

  • Add rust as a build dependency for Node 26 builds.
  • Replace --with-intl=system-icu with --with-intl=full-icu and provide a pinned ICU 78.3 source resource via --with-icu-source.
  • Add a test assertion that globalThis.Temporal.Now.zonedDateTimeISO is present.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread Formula/n/node.rb Outdated
@@ -96,13 +101,15 @@ def install

# Ensure Homebrew deps are used
@Leask Leask force-pushed the codex/node-temporal-support branch from e0cb625 to aceb5e3 Compare May 9, 2026 02:05
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e0cb6251f1

ℹ️ 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".

Comment thread Formula/n/node.rb Outdated
Comment on lines +111 to +112
--with-intl=full-icu
--with-icu-source=#{buildpath/"icu-source/icu"}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Bump the revision for the changed Node build

Because this changes the installed node binary for the same 26.0.0 version by switching ICU modes and enabling Temporal, existing users with the current bottle will not be offered an upgrade unless the formula's revision is incremented. This means the new Temporal support can remain absent for already-installed node 26.0.0 users even after the formula is merged; add a revision bump so Homebrew treats the rebuilt package as an upgrade.

Useful? React with 👍 / 👎.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 9, 2026

Thanks for contributing to Homebrew! 🎉 It looks like you're having trouble with a CI failure. See our contribution guide for help. You may be most interested in the section on dealing with CI failures. You can find the CI logs in the Checks tab of your pull request.

Co-authored-by: Leon Adomaitis <47161699+GunniBusch@users.noreply.github.com>
@Leask Leask force-pushed the codex/node-temporal-support branch from aceb5e3 to c46d2db Compare May 9, 2026 02:35
Copy link
Copy Markdown
Member

@p-linnane p-linnane left a comment

Choose a reason for hiding this comment

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

This feels like an upstream bug that should be fixed rather than building ICU from a resource within this formula.

cc @Homebrew/core

Copy link
Copy Markdown
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Agreed @p-linnane. This might be suitable as a stop-gap but need to have patch submitted and merged upstream first.

@Leask
Copy link
Copy Markdown
Author

Leask commented May 9, 2026

Progress update on the upstream side:

This PR is the current Homebrew-side workaround for enabling Temporal in Node 26 before the upstream V8/Node path lands. It avoids Node's system/shared ICU Temporal-disable path by building Node with full ICU from the Node-pinned ICU source tarball, while keeping the build reproducible by staging the ICU source as a Homebrew resource.

The longer-term upstream fix is being pursued in V8: remove Temporal's dependency on ICU private data-loading APIs (udatamem.h / UDataMemory) and always use V8's generated zoneinfo64_static_data path when Temporal is enabled. Once that lands in V8 and is consumed by Node through a V8 roll or backport, Homebrew should eventually be able to return to the normal system/shared ICU packaging model while still exposing globalThis.Temporal.

So the current state is: this PR keeps Homebrew users unblocked now; the permanent fix should come from V8/Node upstream.

@Leask
Copy link
Copy Markdown
Author

Leask commented May 10, 2026

Update after V8 review feedback on the upstream CL:

V8's feedback is that using ICU4C's zoneinfo64.res data is deliberate: bundled/full ICU already ships the timezone data, and V8 does not want normal i18n builds to duplicate it.

That makes this PR's packaging direction more appropriate, not less. If V8's supported Temporal path is the bundled/full ICU source path, Homebrew should not ship a Node 26 package with Temporal silently trimmed out. Instead, the formula should pin the build to the ICU source/version expected by upstream Node/V8 and produce a complete package.

This PR does that by avoiding the unsupported system/shared ICU Temporal path and using the Node-pinned ICU source as a Homebrew resource with --with-intl=full-icu --with-icu-source=.... That keeps the build reproducible while matching the upstream-supported configuration for Temporal.

So the practical split is:

  • upstream V8 may keep avoiding duplicate timezone data in bundled ICU builds;
  • Node should make the required build mode clearer for packagers;
  • Homebrew should provide a working Node package with Temporal enabled, rather than a reduced build where globalThis.Temporal is missing.

@Leask
Copy link
Copy Markdown
Author

Leask commented May 10, 2026

Another upstream status update:

The upstream positions are not fully aligned yet:

  • V8 does not want to duplicate zoneinfo64.res in normal ICU-enabled builds, because bundled/full ICU already ships that timezone data.
  • Node/system-ICU users reasonably expect system library builds to use ICU public APIs only, not ICU private headers such as udatamem.h.
  • The current V8 Temporal implementation bridges to temporal_rs by needing the raw ICU .res data item plus length, and ICU does not currently expose that exact shape through a stable public API.

The clean long-term fix is probably in ICU: expose a public data-item view/export API for raw ICU data items, so V8 can avoid duplicate timezone data and downstream system-ICU builds can avoid private headers.

That does not help Homebrew users today. For this PR, the short-term compromise is explicit and reproducible: build Node 26 with full ICU from the Node-pinned ICU source tarball instead of system/shared ICU, so the package exposes Temporal rather than silently trimming a major new Node/V8 feature.

I think that is the right packaging tradeoff until upstream V8/Node/ICU converge: ship a working complete Node package now, while tracking the upstream API/design fix separately.

@Bo98
Copy link
Copy Markdown
Member

Bo98 commented May 10, 2026

V8's feedback is that using ICU4C's zoneinfo64.res data is deliberate: bundled/full ICU already ships the timezone data, and V8 does not want normal i18n builds to duplicate it.

...but by using a bundled ICU then they are duplicating the system timezone data, which somewhat defeats the point.

I doubt this is intentional. The build failure when using system ICU is more likely a bug than a design choice.

If V8 really wants to use private API, it is still possible. The symbol does exist in in libicuuc and the relevant name-mapping define does exist in urename.h so you could just write a function prototype for it rather than using the header. Would I recommend this in the long term? Not at all as it's still private API. But it seems like some API changes may need to be made to either ICU4C or ICU4X to support this properly, which may take some time.

Linux distributions are unlikely to want to switch from system ICU. Node upstream are likely aware of this as they made the intentional decision to disable Temporal support for such configurations and I think we should respect that decision until that changes upstream. There is interest in Node to fix this bug, as seen in previous attempts (e.g. https://github.com/nodejs/node/blob/facd71e6856b933894301df00291d24f68e8bfa1/tools/nix/temporal-no-vendored-icu.patch - though that patch is incorrect as it uses udata_getMemory and seems to copy-paste structs and not use them).

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

Labels

long build Set a long timeout for formula testing long dependent tests Set a long timeout for dependent testing rust Rust use is a significant feature of the PR or issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants