Skip to content

Conversation

@LiviaMedeiros
Copy link
Member

Float16Array is expected to become a thing in upcoming major release, and it's already available with --js-float16array runtime v8 flag.

@nodejs-github-botnodejs-github-bot added needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels Apr 14, 2025
@LiviaMedeirosLiviaMedeirosforce-pushed the util-types-isfloat16array branch from 1d0b220 to 2096f20CompareApril 14, 2025 22:14
Copy link
Member

@BridgeARBridgeAR left a comment

Choose a reason for hiding this comment

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

LGTM with the linter issues addressed

@nodejs-github-bot
Copy link
Collaborator

@codecov
Copy link

codecovbot commented Apr 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.24%. Comparing base (b2405e9) to head (e66b702).
Report is 14 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #57879 +/- ## ======================================= Coverage 90.23% 90.24% ======================================= Files 630 630 Lines 185688 185693 +5 Branches 36405 36402 -3 ======================================= + Hits 167559 167578 +19 - Misses 11000 11001 +1 + Partials 7129 7114 -15 
Files with missing linesCoverage Δ
lib/internal/util/types.js100.00% <100.00%> (ø)

... and 22 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

For further information on `napi_create_external`, refer to
[`napi_create_external()`][].
### `util.types.isFloat16Array(value)`
Copy link
Member

Choose a reason for hiding this comment

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

This can be done fully on user land , why are we exposing it?

Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be inconsistent to not expose the 16 bit version next to all others?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure consistency should be enough to expand the API surface. Is there any other reasoning for adding this other than consistency?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

In the scope of this PR, the aim is on having this particular function on node:internal/util/types. It is re-exported as node:util/types as is.

Copy link
Member

Choose a reason for hiding this comment

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

It can't be done in userland easily, though, because you'd have to extract the Symbol.toStringTag getter. Consistency is a pretty strong justification for extending any API surface, on its own merits.

@@ -1,4 +1,5 @@
// Flags: --experimental-vm-modules --expose-internals --allow-natives-syntax
// Flags: --experimental-vm-modules --expose-internals --allow-natives-syntax --js-float16array
// TODO(LiviaMedeiros): once `Float16Array` is unflagged in v8, remove `--js-float16array` above
Copy link
Member

Choose a reason for hiding this comment

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

I also think we shouldn't land this until this becomes stable.

Copy link
Member

Choose a reason for hiding this comment

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

I think doing it the other way around is actually good so that the new method is available as soon as it becomes stable. It will likely not be used before that anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is already stable, it is stage 4 – and if you meant the V8 implementation, that's really not necessary IMO, precisely because we can trust it will follow the spec.

Copy link
Member

@anonriganonrig left a comment

Choose a reason for hiding this comment

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

I don't think we should add this when it is still not exposed globally in V8. We also shouldn't backport this to any other release lines.

@LiviaMedeiros
Copy link
MemberAuthor

I don't think we should add this when it is still not exposed globally in V8.

Why? It is under runtime flag yet, but it already can be used in currently supported release lines. The worst that can happen from using it without the flag is the function returning false (but before that, new Float16Array() would throw anyway).

I understand possible concern of "it is confusing for users if we provide typecheck function for something that doesn't exist without runtime flag", but I can't come up with a scenario where this would actually hurt or be misused.

We also shouldn't backport this to any other release lines.

On the contrary, I'd like to have it as semver-patch and be included/backported in past release lines. The reason is that this would unblock other patches (e.g. #57880).

@ljharb
Copy link
Member

@LiviaMedeiros minor, not patch, since it's adding something :-)

@LiviaMedeirosLiviaMedeiros added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 15, 2025
@nodejs-github-bot

This comment was marked as outdated.

@aduh95aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 15, 2025
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 15, 2025

@aduh95aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Apr 15, 2025
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 16, 2025
@nodejs-github-botnodejs-github-bot merged commit e61937b into nodejs:mainApr 16, 2025
67 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in e61937b

RafaelGSS pushed a commit that referenced this pull request May 1, 2025
PR-URL: #57879 Reviewed-By: Jordan Harband <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
RafaelGSS pushed a commit that referenced this pull request May 2, 2025
PR-URL: #57879 Reviewed-By: Jordan Harband <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
aduh95 pushed a commit that referenced this pull request May 6, 2025
PR-URL: #57879 Reviewed-By: Jordan Harband <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
aduh95 pushed a commit that referenced this pull request May 6, 2025
PR-URL: #57879 Reviewed-By: Jordan Harband <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
RafaelGSS pushed a commit that referenced this pull request May 14, 2025
PR-URL: #57879 Reviewed-By: Jordan Harband <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
aduh95 pushed a commit that referenced this pull request May 16, 2025
PR-URL: #57879 Reviewed-By: Jordan Harband <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
aduh95 pushed a commit that referenced this pull request May 17, 2025
PR-URL: #57879 Reviewed-By: Jordan Harband <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
aduh95 pushed a commit that referenced this pull request May 17, 2025
PR-URL: #57879 Reviewed-By: Jordan Harband <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
aduh95 pushed a commit that referenced this pull request May 17, 2025
PR-URL: #57879 Reviewed-By: Jordan Harband <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
aduh95 pushed a commit that referenced this pull request May 18, 2025
PR-URL: #57879 Reviewed-By: Jordan Harband <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
nodejs-github-bot added a commit that referenced this pull request May 19, 2025
Notable changes: deps: * update timezone to 2025b (Node.js GitHub Bot) #57857 doc: * add dario-piotrowicz to collaborators (Dario Piotrowicz) #58102 * (SEMVER-MINOR) graduate multiple experimental apis (James M Snell) #57765 esm: * (SEMVER-MINOR) graduate import.meta properties (James M Snell) #58011 * (SEMVER-MINOR) support top-level Wasm without package type (Guy Bedford) #57610 sqlite: * (SEMVER-MINOR) add StatementSync.prototype.columns() (Colin Ihrig) #57490 src: * (SEMVER-MINOR) set default config as `node.config.json` (Marco Ippolito) #57171 * (SEMVER-MINOR) create `THROW_ERR_OPTIONS_BEFORE_BOOTSTRAPPING` (Marco Ippolito) #57016 * (SEMVER-MINOR) add config file support (Marco Ippolito) #57016 * (SEMVER-MINOR) add ExecutionAsyncId getter for any Context (Attila Szegedi) #57820 stream: * (SEMVER-MINOR) preserve AsyncLocalStorage context in finished() (Gürgün Dayıoğlu) #57865 util: * (SEMVER-MINOR) add `types.isFloat16Array()` (Livia Medeiros) #57879 worker: * (SEMVER-MINOR) add worker.getHeapStatistics() (Matteo Collina) #57888 PR-URL: #58388
aduh95 pushed a commit that referenced this pull request May 19, 2025
PR-URL: #57879 Reviewed-By: Jordan Harband <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
aduh95 added a commit that referenced this pull request May 19, 2025
Notable changes: deps: * update timezone to 2025b (Node.js GitHub Bot) #57857 doc: * add dario-piotrowicz to collaborators (Dario Piotrowicz) #58102 * (SEMVER-MINOR) graduate multiple experimental apis (James M Snell) #57765 esm: * (SEMVER-MINOR) graduate import.meta properties (James M Snell) #58011 * (SEMVER-MINOR) support top-level Wasm without package type (Guy Bedford) #57610 sqlite: * (SEMVER-MINOR) add StatementSync.prototype.columns() (Colin Ihrig) #57490 src: * (SEMVER-MINOR) set default config as `node.config.json` (Marco Ippolito) #57171 * (SEMVER-MINOR) create `THROW_ERR_OPTIONS_BEFORE_BOOTSTRAPPING` (Marco Ippolito) #57016 * (SEMVER-MINOR) add config file support (Marco Ippolito) #57016 * (SEMVER-MINOR) add ExecutionAsyncId getter for any Context (Attila Szegedi) #57820 stream: * (SEMVER-MINOR) preserve AsyncLocalStorage context in finished() (Gürgün Dayıoğlu) #57865 util: * (SEMVER-MINOR) add `types.isFloat16Array()` (Livia Medeiros) #57879 worker: * (SEMVER-MINOR) add worker.getHeapStatistics() (Matteo Collina) #57888 PR-URL: TODO
aduh95 added a commit that referenced this pull request May 19, 2025
Notable changes: deps: * update timezone to 2025b (Node.js GitHub Bot) #57857 doc: * add dario-piotrowicz to collaborators (Dario Piotrowicz) #58102 * (SEMVER-MINOR) graduate multiple experimental apis (James M Snell) #57765 esm: * (SEMVER-MINOR) graduate import.meta properties (James M Snell) #58011 * (SEMVER-MINOR) support top-level Wasm without package type (Guy Bedford) #57610 sqlite: * (SEMVER-MINOR) add StatementSync.prototype.columns() (Colin Ihrig) #57490 src: * (SEMVER-MINOR) set default config as `node.config.json` (Marco Ippolito) #57171 * (SEMVER-MINOR) create `THROW_ERR_OPTIONS_BEFORE_BOOTSTRAPPING` (Marco Ippolito) #57016 * (SEMVER-MINOR) add config file support (Marco Ippolito) #57016 * (SEMVER-MINOR) add ExecutionAsyncId getter for any Context (Attila Szegedi) #57820 stream: * (SEMVER-MINOR) preserve AsyncLocalStorage context in finished() (Gürgün Dayıoğlu) #57865 util: * (SEMVER-MINOR) add `types.isFloat16Array()` (Livia Medeiros) #57879 worker: * (SEMVER-MINOR) add worker.getHeapStatistics() (Matteo Collina) #57888 PR-URL: #58388
aduh95 added a commit that referenced this pull request May 19, 2025
Notable changes: deps: * update timezone to 2025b (Node.js GitHub Bot) #57857 doc: * add dario-piotrowicz to collaborators (Dario Piotrowicz) #58102 * (SEMVER-MINOR) graduate multiple experimental apis (James M Snell) #57765 esm: * (SEMVER-MINOR) graduate import.meta properties (James M Snell) #58011 * (SEMVER-MINOR) support top-level Wasm without package type (Guy Bedford) #57610 sqlite: * (SEMVER-MINOR) add StatementSync.prototype.columns() (Colin Ihrig) #57490 src: * (SEMVER-MINOR) set default config as `node.config.json` (Marco Ippolito) #57171 * (SEMVER-MINOR) create `THROW_ERR_OPTIONS_BEFORE_BOOTSTRAPPING` (Marco Ippolito) #57016 * (SEMVER-MINOR) add config file support (Marco Ippolito) #57016 * (SEMVER-MINOR) add ExecutionAsyncId getter for any Context (Attila Szegedi) #57820 stream: * (SEMVER-MINOR) preserve AsyncLocalStorage context in finished() (Gürgün Dayıoğlu) #57865 util: * (SEMVER-MINOR) add `types.isFloat16Array()` (Livia Medeiros) #57879 worker: * (SEMVER-MINOR) add worker.getHeapStatistics() (Matteo Collina) #57888 PR-URL: #58388
aduh95 added a commit to aduh95/node that referenced this pull request May 20, 2025
Notable changes: deps: * update timezone to 2025b (Node.js GitHub Bot) nodejs#57857 doc: * add dario-piotrowicz to collaborators (Dario Piotrowicz) nodejs#58102 * (SEMVER-MINOR) graduate multiple experimental apis (James M Snell) nodejs#57765 esm: * (SEMVER-MINOR) graduate import.meta properties (James M Snell) nodejs#58011 * (SEMVER-MINOR) support top-level Wasm without package type (Guy Bedford) nodejs#57610 sqlite: * (SEMVER-MINOR) add StatementSync.prototype.columns() (Colin Ihrig) nodejs#57490 src: * (SEMVER-MINOR) set default config as `node.config.json` (Marco Ippolito) nodejs#57171 * (SEMVER-MINOR) create `THROW_ERR_OPTIONS_BEFORE_BOOTSTRAPPING` (Marco Ippolito) nodejs#57016 * (SEMVER-MINOR) add config file support (Marco Ippolito) nodejs#57016 * (SEMVER-MINOR) add ExecutionAsyncId getter for any Context (Attila Szegedi) nodejs#57820 stream: * (SEMVER-MINOR) preserve AsyncLocalStorage context in finished() (Gürgün Dayıoğlu) nodejs#57865 util: * (SEMVER-MINOR) add `types.isFloat16Array()` (Livia Medeiros) nodejs#57879 worker: * (SEMVER-MINOR) add worker.getHeapStatistics() (Matteo Collina) nodejs#57888 PR-URL: nodejs#58388
aduh95 added a commit that referenced this pull request May 21, 2025
Notable changes: deps: * update timezone to 2025b (Node.js GitHub Bot) #57857 doc: * add dario-piotrowicz to collaborators (Dario Piotrowicz) #58102 * (SEMVER-MINOR) graduate multiple experimental apis (James M Snell) #57765 esm: * (SEMVER-MINOR) graduate import.meta properties (James M Snell) #58011 * (SEMVER-MINOR) support top-level Wasm without package type (Guy Bedford) #57610 sqlite: * (SEMVER-MINOR) add StatementSync.prototype.columns() (Colin Ihrig) #57490 src: * (SEMVER-MINOR) set default config as `node.config.json` (Marco Ippolito) #57171 * (SEMVER-MINOR) create `THROW_ERR_OPTIONS_BEFORE_BOOTSTRAPPING` (Marco Ippolito) #57016 * (SEMVER-MINOR) add config file support (Marco Ippolito) #57016 * (SEMVER-MINOR) add ExecutionAsyncId getter for any Context (Attila Szegedi) #57820 stream: * (SEMVER-MINOR) preserve AsyncLocalStorage context in finished() (Gürgün Dayıoğlu) #57865 util: * (SEMVER-MINOR) add `types.isFloat16Array()` (Livia Medeiros) #57879 worker: * (SEMVER-MINOR) add worker.getHeapStatistics() (Matteo Collina) #57888 PR-URL: #58388
@ghostghost mentioned this pull request Jun 8, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.needs-ciPRs that need a full CI run.semver-minorPRs that contain new features and should be released in the next minor version.utilIssues and PRs related to the built-in util module.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@LiviaMedeiros@nodejs-github-bot@ljharb@jasnell@lpinca@anonrig@BridgeAR@aduh95