-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
util: add types.isFloat16Array()
#57879
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
util: add types.isFloat16Array()
#57879
Conversation
1d0b220
to
2096f20
Compare
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.
LGTM with the linter issues addressed
Codecov ReportAll modified and coverable lines are covered by tests ✅
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
🚀 New features to boost your workflow:
|
@@ -3102,6 +3102,23 @@ types.isExternal(new String('foo')); // returns false | |||
For further information on `napi_create_external`, refer to | |||
[`napi_create_external()`][]. | |||
|
|||
### `util.types.isFloat16Array(value)` |
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.
This can be done fully on user land , why are we exposing it?
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.
Wouldn't it be inconsistent to not expose the 16 bit version next to all others?
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.
I'm not sure consistency should be enough to expand the API surface. Is there any other reasoning for adding this other than consistency?
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.
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.
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.
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 |
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.
I also think we shouldn't land this until this becomes stable.
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.
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.
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.
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.
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.
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.
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 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.
On the contrary, I'd like to have it as |
@LiviaMedeiros minor, not patch, since it's adding something :-) |
This comment was marked as outdated.
This comment was marked as outdated.
Landed in e61937b |
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]>
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]>
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]>
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]>
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]>
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]>
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]>
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]>
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]>
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]>
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
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]>
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
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
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
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
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
Float16Array
is expected to become a thing in upcoming major release, and it's already available with--js-float16array
runtime v8 flag.