lib: add internal Temporal utils#63312
Conversation
Signed-off-by: Renegade334 <[email protected]>
50da00b to
907a6eb
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #63312 +/- ##
========================================
Coverage 90.05% 90.05%
========================================
Files 714 715 +1
Lines 225247 225438 +191
Branches 42578 42616 +38
========================================
+ Hits 202842 203019 +177
- Misses 14181 14195 +14
Partials 8224 8224
🚀 New features to boost your workflow:
|
|
|
||
| exports.hasTemporal = true; | ||
|
|
||
| for (const { 0: name, 1: brandedGetter } of constructors.entries()) { |
There was a problem hiding this comment.
nit
| for (const { 0: name, 1: brandedGetter } of constructors.entries()) { | |
| for (const { 0: name, 1: brandedGetter } of constructors) { |
| try { | ||
| FunctionPrototypeCall(getter, value); | ||
| return true; | ||
| } catch { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
This is going to be quite bad for performance, we should probably add a comment about that. V8 seems to have utilities to check that, but looks like they're not exposed :/
node/deps/v8/src/objects/js-date-time-format.cc
Lines 841 to 845 in 676e9da
There was a problem hiding this comment.
I'm hoping that these will very much be a stop-gap until some API methods get implemented. Having said that, @panva did find recently that using this sort of speculative method check in the JS layer was surprisingly faster than invoking the C++ typecheck call in benchmarks on main (that case was for ArrayBuffers specifically).
There was a problem hiding this comment.
That's interesting! Still with try/catch? Another approach would be to use @@toStringTag (defined in e.g. https://tc39.es/proposal-temporal/#sec-temporal.instant.prototype-%symbol.tostringtag%). btw shouldn't we move this to internal/util/types?
There was a problem hiding this comment.
That's interesting! Still with
try/catch?
Same approach, yes. Would have to look for the exact benchmark.
Another approach would be to use
@@toStringTag(defined in e.g. https://tc39.es/proposal-temporal/#sec-temporal.instant.prototype-%symbol.tostringtag%).
It's a property, not a getter, on the Temporal prototypes.
btw shouldn't we move this to
internal/util/types?
Pretty sure that's snapshotted. Additionally, those methods are automatically exposed to userland, and I don't think this should be considered a stable enough approach for that.
Spun out from #63154 to garner comments. See that PR for an example of how this might be used in practice, and #60789 for an example of a feature that could make use of it.
Integrating Temporal into Node.js APIs is currently made challenging by the fact that it is not available at bootstrapping, meaning no primordials and no user-resistant way of accessing the Temporal globals. Unlike other similar features like SharedArrayBuffer, Temporal is very commonly polyfilled across the ecosystem, so running into a non-builtin implementation in the global namespace is a real possibility.
The likelihood is that the combination of build-time and run-time flags that govern this behaviour is not going away any time soon, so in the meantime, we could really do with something more robust than seeking a property on
globalThisand hoping for the best.This introduces a
internal/util/temporalmodule which is initialized during the pre-execution phase, to mitigate against the user mutability issue. The module exports a number of utilities:hasTemporalproperty for determining whether Node.js is running with builtin Temporal supportisTemporalDuration(),isTemporalInstant()etc.) which perform realm-agnostic type checking for Temporal builtin objects, available regardless of whether Temporal support is enabledObviously this isn't snapshottable, so unlike primordials, there is a small per-run overhead here. Worthwhile?