Skip to content

Conversation

@nodejs-github-bot
Copy link
Collaborator

This is an automated update of temporal to 0.1.2.

@nodejs-github-bot nodejs-github-bot added the dependencies Pull requests that update a dependency file. label Nov 30, 2025
@nodejs-github-bot
Copy link
Collaborator Author

Review requested:

  • @nodejs/security-wg

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Nov 30, 2025
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

IIUC we don't want to update before we update V8 to 14.4

@targos
Copy link
Member

targos commented Dec 1, 2025

Why not? It's a wip feature, behind a flag and our minimal tests don't fail.

@aduh95
Copy link
Contributor

aduh95 commented Dec 1, 2025

The two versions are not compatible AFAIK, we would need to either cherry-pick the commits that landed on V8 to make it compatible (at least v8/v8@0cbecc1), accept a broken state, or simply not land this until we update the version of V8. The former seems hardly worth it (as you said we don't use it officially anyway), so the latter seems the best course of action as I don't understand what would be the upside of purposefully land something that we know to be broken

@targos
Copy link
Member

targos commented Dec 1, 2025

Maybe we should reconsider having a script to auto-update this dependency if it's tightly coupled with V8

@targos
Copy link
Member

targos commented Dec 1, 2025

Or said another way: change the V8 update script so it also updates temporal with right version.

@aduh95
Copy link
Contributor

aduh95 commented Dec 1, 2025

That would be ideal yes, unfortunately I found it harder than I anticipated to set up. I think V8 14.4 should be buildable with that version, but yeah it could become a maintenance burden

@legendecas
Copy link
Member

legendecas commented Dec 3, 2025

But surprisingly, the CI on this PR is green? definitely, we don't have any temporal CI.

@richardlau
Copy link
Member

Maybe we should reconsider having a script to auto-update this dependency if it's tightly coupled with V8

I had some time today to look at temporal and what we might need to do to vendor everything into the Node.js source tree (currently building deps/temporal via --v8-enable-temporal-support pulls down a bunch of dependencies via cargo from the Internet). The official way is cargo vendor (and then at build time passing --offline to cargo).

If I run that in deps/temporal, I end up with a vendor subdirectory that is 119MB in size... which is not too far off Google's own vendored rust crates in https://chromium.googlesource.com/chromium/src/third_party/rust/+/4d93511ebaceb09ebdd83c8876a4a936b75fa04d (from

node/deps/v8/DEPS

Lines 508 to 509 in 746c3c2

'third_party/rust':
Var('chromium_url') + '/chromium/src/third_party/rust' + '@' + '4d93511ebaceb09ebdd83c8876a4a936b75fa04d',
) which is 157MB.

Point here being that if we think the version of temporal is going to be coupled to the V8 version, the easiest solution (at the cost of disk space) would be to vendor in V8's third_party/rust dependency and have that update (via @node-core/utils) when V8 is.

Another advantage to using Google's vendored crates is that we would pick up patches from https://chromium.googlesource.com/chromium/src/third_party/rust/+/4d93511ebaceb09ebdd83c8876a4a936b75fa04d/chromium_crates_io/patches/. There isn't one for temporal at the commit for the version of V8 we're currently using, but there is one on their main so the possibility is real.

@aduh95
Copy link
Contributor

aduh95 commented Dec 3, 2025

157 MiB sounds like a lot! Looking at https://github.com/nodejs/node/actions/runs/19791503862?pr=60897, currently the source code + vendored deps is about 107MiB gzipped (the source code without sharable deps is about 48 MiB gzipped, according to https://github.com/nodejs/node/actions/runs/19903600039). If that's the solution we go with, it wouldn't make sense to backport it to v25.x.

@legendecas
Copy link
Member

chromium/src/third_party/rust/ does not include every crate that cargo vendor would do. Like windows_aarch64_gnullvm, windows_i686_gnullvm, windows_x86_64_gnu etc, which consist a large portion of the size cargo vendor creates. We should probably do the similar trimming on the vendored sources.

Refs: https://chromium.googlesource.com/chromium/src/+/refs/heads/main/tools/crates/gnrt/removed_crate.md

@richardlau
Copy link
Member

@legendecas Yeah, the trimming that Chromium already do is another point in favour of using chromium/src/third_party/rust/ instead of trying to replicate that (and any patching) ourselves.

Planning for when eventually temporal is enabled by default, we'll need to vendor (either ourselves or reusing Chromium's chromium/src/third_party/rust/). One question is whether we want to be able to update temporal_capi independently of V8? We do for, e.g. zlib, icu, but do not for e.g. simdutf.

@legendecas
Copy link
Member

The issue with chromium/src/third_party/rust/ is that it is not dedicated for V8 only dependencies. It also includes dependencies used by Chromium and not going to be used by Node.js (in the foreseeable future), like png, qr_code, font libraries read-fonts and skrifa (link).

We can not stop chromium from adding more dependencies we don't need here in Node.js to chromium/src/third_party/rust/.

@targos
Copy link
Member

targos commented Dec 4, 2025

We can enumerate the dependencies that are needed for temporal_capi and gitignore the other ones.

@Manishearth
Copy link

We can enumerate the dependencies that are needed for temporal_capi and gitignore the other ones.

Yeah, this is what I would recommend. When temporal_rs updates there is a chance that more dependencies will be needed, but that can be pretty easily noticed by CI.

The way to get this list easily is to take the Temporal-relevant dependencies from Chrome's Cargo.toml, stick them in a crate, and run cargo tree -e normal

[dependencies.temporal_capi]
version = "0.1.2"
features = ["zoneinfo64"]

[dependencies.temporal_rs]
version = "0.1.2"
default-features = false
# This is necessary to enable a spec-compliance quirk
features = ["float64_representable_durations"]
cargo tree output
$ cargo tree -e normal
foo v0.1.0 (/home/manishearth/sand/crates/foo)
├── temporal_capi v0.1.2
│   ├── diplomat v0.14.0 (proc-macro)
│   │   ├── diplomat_core v0.14.0
│   │   │   ├── proc-macro2 v1.0.103
│   │   │   │   └── unicode-ident v1.0.20
│   │   │   ├── quote v1.0.41
│   │   │   │   └── proc-macro2 v1.0.103 (*)
│   │   │   ├── serde v1.0.228
│   │   │   │   ├── serde_core v1.0.228
│   │   │   │   └── serde_derive v1.0.228 (proc-macro)
│   │   │   │       ├── proc-macro2 v1.0.103 (*)
│   │   │   │       ├── quote v1.0.41 (*)
│   │   │   │       └── syn v2.0.108
│   │   │   │           ├── proc-macro2 v1.0.103 (*)
│   │   │   │           ├── quote v1.0.41 (*)
│   │   │   │           └── unicode-ident v1.0.20
│   │   │   ├── smallvec v1.15.1
│   │   │   ├── strck v1.0.0
│   │   │   │   └── unicode-ident v1.0.20
│   │   │   └── syn v2.0.108 (*)
│   │   ├── proc-macro2 v1.0.103 (*)
│   │   ├── quote v1.0.41 (*)
│   │   └── syn v2.0.108 (*)
│   ├── diplomat-runtime v0.14.0
│   ├── icu_calendar v2.1.0
│   │   ├── calendrical_calculations v0.2.3
│   │   │   ├── core_maths v0.1.1
│   │   │   │   └── libm v0.2.15
│   │   │   └── displaydoc v0.2.5 (proc-macro)
│   │   │       ├── proc-macro2 v1.0.103 (*)
│   │   │       ├── quote v1.0.41 (*)
│   │   │       └── syn v2.0.108 (*)
│   │   ├── displaydoc v0.2.5 (proc-macro) (*)
│   │   ├── icu_calendar_data v2.1.1
│   │   ├── icu_locale v2.1.1
│   │   │   ├── icu_collections v2.1.1
│   │   │   │   ├── displaydoc v0.2.5 (proc-macro) (*)
│   │   │   │   ├── potential_utf v0.1.4
│   │   │   │   │   ├── writeable v0.6.2
│   │   │   │   │   └── zerovec v0.11.5
│   │   │   │   │       ├── yoke v0.8.1
│   │   │   │   │       │   ├── stable_deref_trait v1.2.1
│   │   │   │   │       │   ├── yoke-derive v0.8.1 (proc-macro)
│   │   │   │   │       │   │   ├── proc-macro2 v1.0.103 (*)
│   │   │   │   │       │   │   ├── quote v1.0.41 (*)
│   │   │   │   │       │   │   ├── syn v2.0.108 (*)
│   │   │   │   │       │   │   └── synstructure v0.13.2
│   │   │   │   │       │   │       ├── proc-macro2 v1.0.103 (*)
│   │   │   │   │       │   │       ├── quote v1.0.41 (*)
│   │   │   │   │       │   │       └── syn v2.0.108 (*)
│   │   │   │   │       │   └── zerofrom v0.1.6
│   │   │   │   │       │       └── zerofrom-derive v0.1.6 (proc-macro)
│   │   │   │   │       │           ├── proc-macro2 v1.0.103 (*)
│   │   │   │   │       │           ├── quote v1.0.41 (*)
│   │   │   │   │       │           ├── syn v2.0.108 (*)
│   │   │   │   │       │           └── synstructure v0.13.2 (*)
│   │   │   │   │       ├── zerofrom v0.1.6 (*)
│   │   │   │   │       └── zerovec-derive v0.11.2 (proc-macro)
│   │   │   │   │           ├── proc-macro2 v1.0.103 (*)
│   │   │   │   │           ├── quote v1.0.41 (*)
│   │   │   │   │           └── syn v2.0.108 (*)
│   │   │   │   ├── yoke v0.8.1 (*)
│   │   │   │   ├── zerofrom v0.1.6 (*)
│   │   │   │   └── zerovec v0.11.5 (*)
│   │   │   ├── icu_locale_core v2.1.1
│   │   │   │   ├── displaydoc v0.2.5 (proc-macro) (*)
│   │   │   │   ├── litemap v0.8.1
│   │   │   │   ├── tinystr v0.8.2
│   │   │   │   │   ├── displaydoc v0.2.5 (proc-macro) (*)
│   │   │   │   │   └── zerovec v0.11.5 (*)
│   │   │   │   ├── writeable v0.6.2
│   │   │   │   └── zerovec v0.11.5 (*)
│   │   │   ├── icu_locale_data v2.1.1
│   │   │   ├── icu_provider v2.1.1
│   │   │   │   ├── displaydoc v0.2.5 (proc-macro) (*)
│   │   │   │   ├── icu_locale_core v2.1.1 (*)
│   │   │   │   ├── stable_deref_trait v1.2.1
│   │   │   │   ├── writeable v0.6.2
│   │   │   │   ├── yoke v0.8.1 (*)
│   │   │   │   ├── zerofrom v0.1.6 (*)
│   │   │   │   ├── zerotrie v0.2.3
│   │   │   │   │   └── displaydoc v0.2.5 (proc-macro) (*)
│   │   │   │   └── zerovec v0.11.5 (*)
│   │   │   ├── potential_utf v0.1.4 (*)
│   │   │   ├── tinystr v0.8.2 (*)
│   │   │   └── zerovec v0.11.5 (*)
│   │   ├── icu_locale_core v2.1.1 (*)
│   │   ├── icu_provider v2.1.1 (*)
│   │   ├── tinystr v0.8.2 (*)
│   │   └── zerovec v0.11.5 (*)
│   ├── icu_locale v2.1.1 (*)
│   ├── num-traits v0.2.19
│   ├── temporal_rs v0.1.2
│   │   ├── core_maths v0.1.1 (*)
│   │   ├── icu_calendar v2.1.0 (*)
│   │   ├── icu_locale v2.1.1 (*)
│   │   ├── ixdtf v0.6.4
│   │   ├── num-traits v0.2.19
│   │   ├── timezone_provider v0.1.2
│   │   │   ├── tinystr v0.8.2 (*)
│   │   │   ├── zerotrie v0.2.3 (*)
│   │   │   ├── zerovec v0.11.5 (*)
│   │   │   └── zoneinfo64 v0.2.1
│   │   │       ├── calendrical_calculations v0.2.3 (*)
│   │   │       ├── icu_locale_core v2.1.1 (*)
│   │   │       ├── potential_utf v0.1.4 (*)
│   │   │       ├── resb v0.1.1
│   │   │       │   ├── potential_utf v0.1.4 (*)
│   │   │       │   └── serde_core v1.0.228
│   │   │       └── serde v1.0.228
│   │   │           ├── serde_core v1.0.228
│   │   │           └── serde_derive v1.0.228 (proc-macro) (*)
│   │   ├── tinystr v0.8.2 (*)
│   │   └── writeable v0.6.2
│   ├── timezone_provider v0.1.2 (*)
│   ├── writeable v0.6.2
│   └── zoneinfo64 v0.2.1 (*)
└── temporal_rs v0.1.2 (*)

Currently, that boils down to this list of crates: calendrical_calculations, core_maths, diplomat, diplomat_core, diplomat-runtime, displaydoc, icu_calendar, icu_calendar_data, icu_collections, icu_locale, icu_locale_core, icu_locale_data, icu_provider, ixdtf, libm, litemap, num-traits, potential_utf, proc-macro2, quote, resb, serde, serde_core, serde_derive, smallvec, stable_deref_trait, strck, syn, synstructure, temporal_capi, temporal_rs, timezone_provider, tinystr, unicode-ident, writeable, yoke, yoke-derive, zerofrom, zerofrom-derive, zerotrie, zerovec, zerovec-derive, zoneinfo64.

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

Labels

dependencies Pull requests that update a dependency file. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants