node: enable Temporal support#281626
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the Homebrew node formula to build Node.js with V8 Temporal support enabled, aligning the bottle’s runtime behavior with upstream Node.js v26 (i.e., globalThis.Temporal is available).
Changes:
- Add
--v8-enable-temporal-supportto Node’s./configurearguments. - Extend the formula test to assert
Temporal.Now.zonedDateTimeISOis exposed as a function.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ebf4d0df3
ℹ️ 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".
| --with-intl=system-icu | ||
| --shared | ||
| --openssl-use-def-ca-store | ||
| --v8-enable-temporal-support |
There was a problem hiding this comment.
Bump revision when changing build output
Because this adds a configure flag that changes the installed node binary's behavior while leaving the formula at 26.0.0 with no revision, users who already have the current bottle installed won't be considered outdated and will keep the build where globalThis.Temporal is undefined. Please add a revision bump so existing installs are upgraded to the rebuilt bottle with Temporal enabled.
Useful? React with 👍 / 👎.
|
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. |
8ebf4d0 to
47e7259
Compare
| # TODO: Try to devendor these libraries. | ||
| # - `--shared-gtest` is only used for building the test suite, which we don't run here. | ||
| # - `--shared-simdutf` seems to result in build failures. | ||
| # - `--shared-temporal_capi` is only used when building with `--v8-enable-temporal-support` |
| url "https://nodejs.org/dist/v26.0.0/node-v26.0.0.tar.xz" | ||
| sha256 "fcb5e5c06a5c2ec9e669801248657aafaa2291f8760dac7bfb639f878318c592" | ||
| license "MIT" | ||
| revision 1 |
There was a problem hiding this comment.
We don't need to force an update on everyone. You can drop this.
There was a problem hiding this comment.
I slightly disagree, because in the context of Node semantics, the absence of Temporal can reasonably be considered a non-standard build. Is the new version that Temporal is enabled by default? Should we update everyone’s incorrect version through a revision?
There was a problem hiding this comment.
I will remove the revision to pass the subsequent review for now.
2a56b89 to
9d31eb5
Compare
Co-authored-by: Leon Adomaitis <47161699+GunniBusch@users.noreply.github.com>
9d31eb5 to
aff8a19
Compare
|
Not working because:
|
|
Thanks for checking this. CI confirms the blocker: Node currently disables Temporal when built against a shared ICU library, and the Homebrew formula intentionally builds with system/shared ICU. So this PR cannot enable Temporal with a small formula change without moving Node away from shared ICU, which is too broad for this fix. This should be revisited after the upstream Node issue is resolved: nodejs/node#62676 |
|
I tried a build-time ICU source approach after closing this, but this PR cannot be reopened because the head branch was force-pushed after closure. I will open a replacement PR with the corrected approach. The corrected approach avoids system/shared ICU for Node 26 and vendors the Node-pinned ICU 78.3 source tarball as a Homebrew resource, then passes |
|
Replacement PR opened with the build-time ICU source approach: #281707 |
Temporal.Now.zonedDateTimeISOFixes #281625.
The upstream Node.js v26.0.0 darwin-arm64 binary has
process.config.variables.v8_enable_temporal_support === 1and exposesglobalThis.Temporal, while the current Homebrew bottle reports0and leavesglobalThis.Temporalundefined.Verification:
brew style nodebrew audit --strict nodeI did not run
brew test nodeagainst the current installed bottle because the bottle was built before this configure flag change and now correctly fails the new Temporal assertion. The PR CI/source build should verify the rebuilt formula.