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.

Sign up for GitHub

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
+27 −0
Open

lib: unflag experimental Temporal #57128

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions doc/api/globals.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1075,6 +1075,17 @@ added: v17.0.0

The WHATWG `DOMException` class. See [`DOMException`][] for more details.

## `Temporal`

<!-- YAML
added: REPLACEME
-->

> Stability: 1 - Experimental.

The `Temporal` global is a new language level API for working with dates and
times.

Comment on lines +1078 to +1088
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?

## `TextDecoder`

<!-- YAML
Expand Down
7 changes: 7 additions & 0 deletions src/node.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -758,6 +758,13 @@ static ExitCode ProcessGlobalArgsInternal(std::vector<std::string>* args,
v8_args.emplace_back("--harmony-import-attributes");
}

// TODO(jasnell): remove this when the harmony-temporal flag
// is removed in V8.
if (std::find(v8_args.begin(), v8_args.end(), "--no-harmony-temporal") ==
v8_args.end()) {
v8_args.emplace_back("--harmony-temporal");
}

auto env_opts = per_process::cli_options->per_isolate->per_env;
if (std::find(v8_args.begin(), v8_args.end(),
"--abort-on-uncaught-exception") != v8_args.end() ||
Expand Down
9 changes: 9 additions & 0 deletions test/parallel/test-temporal-present.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
'use strict';

require('../common');

const { ok } = require('assert');

// 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.

Loading