-
-
Notifications
You must be signed in to change notification settings - Fork 33.9k
deps: update temporal to 0.1.2 #60897
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Review requested:
|
aduh95
left a comment
There was a problem hiding this 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
|
Why not? It's a wip feature, behind a flag and our minimal tests don't fail. |
|
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 |
|
Maybe we should reconsider having a script to auto-update this dependency if it's tightly coupled with V8 |
|
Or said another way: change the V8 update script so it also updates temporal with right version. |
|
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 |
|
|
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 If I run that in Lines 508 to 509 in 746c3c2
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 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 |
|
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. |
|
Refs: https://chromium.googlesource.com/chromium/src/+/refs/heads/main/tools/crates/gnrt/removed_crate.md |
|
@legendecas Yeah, the trimming that Chromium already do is another point in favour of using Planning for when eventually temporal is enabled by default, we'll need to vendor (either ourselves or reusing Chromium's |
|
The issue with We can not stop chromium from adding more dependencies we don't need here in Node.js to |
|
We can enumerate the dependencies that are needed for |
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 outputCurrently, 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. |
This is an automated update of temporal to 0.1.2.