Skip to content
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

lib: unflag experimental Temporal #57128

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Feb 18, 2025

Fixes #57127

However, we might want to let this sit for a bit per @ljharb's comments in #57127. I'm happy to let this one sit for a bit (likely can land in main immediately after cutting 24.0.0 in April)

Since it adds a new global, this PR must be semver-major

@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 18, 2025
@jasnell jasnell requested a review from ljharb February 18, 2025 19:43
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Feb 18, 2025
@jasnell jasnell added the blocked PRs that are blocked by other issues or PRs. label Feb 18, 2025
@jasnell
Copy link
Member Author

jasnell commented Feb 18, 2025

Marking as blocked until we resolve questions around timing

@jasnell jasnell force-pushed the jasnell/experimental-temporal-unflagged branch from ad64f38 to 8516196 Compare February 18, 2025 19:49
@cjihrig
Copy link
Contributor

cjihrig commented Feb 18, 2025

I think we should either:

  • Wait until it is fully unflagged in V8.
  • Land it experimentally in Node 24.

Since this is semver major, my preference would be landing it experimentally in Node 24. Otherwise, users have to wait until 2028 for it to be available on all supported release lines. We can knock a year off of that by getting it into v24.

@jasnell
Copy link
Member Author

jasnell commented Feb 18, 2025

Note that it is already possible to use Temporal in Node.js using the --harmony-temporal flag. This PR just unflags it. The documentation update clarifies that it remains experimental.

@cjihrig
Copy link
Contributor

cjihrig commented Feb 18, 2025

Of course. I should have said unflagged instead of available in my original comment. CLI flags are the biggest hurdle to actually using something. Experimental warnings are annoying but easy to silence/ignore. Experimental documentation might as well be a tag on a mattress 😄.


// Test verifying that Temporal is present in the global scope

ok(globalThis.Temporal);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the global leak detection in common be updated to include this global if it is present?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, good catch. Forgot about that.

Comment on lines +1078 to +1088
## `Temporal`

<!-- YAML
added: REPLACEME
-->

> Stability: 1 - Experimental.
The `Temporal` global is a new language level API for working with dates and
times.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it makes sense to list it here, since it's not our implementation, it is a built-in object (we do not list JSON or Atomics either)

node/doc/api/globals.md

Lines 18 to 20 in d1f8ccb

The objects listed here are specific to Node.js. There are [built-in objects][]
that are part of the JavaScript language itself, which are also globally
accessible.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, I wasn't sure how else to indicate that it would still be experimental tho. We don't really have another place to list it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I guess we don't really have a precedent for an experimental global that's not implemented by us. What do we think about having an experimental runtime warning instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

A warning is certainly a possibility here. I'm not sure if that should be in replacement for the documentation tho. Will have to think about it.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about mentioning the flag (or its inverse, depending on how this is going to be exposed) under cli.md#useful-v8-options?

@aduh95
Copy link
Contributor

aduh95 commented Feb 18, 2025

We should add it on the list of no-restricted-globals in lib/eslint.config_partial.mjs, with a comment explaining why it's not available in primordials (i.e. because of the runtime flag).

Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

I'm +1 on landing this as experimental on v24

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked PRs that are blocked by other issues or PRs. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Let's enable Temporal by default
6 participants