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: handle Float16Array in node:v8 serdes #55996

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

bartlomieju
Copy link

This commit adds support for Float16Array in node:v8 serde APIs.

While Float16Array is not available as a global yet (pending stabilization in V8),
I "reserved" the next available index for serde purposes so other runtimes, like Deno,
that already support Float16Array, can support it in node:v8 serde APIs
in a future compatible way.

Closes #55574.

@bartlomieju
Copy link
Author

CC @bnoordhuis @targos

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Nov 26, 2024
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

The linter is failing (make lint-js).

test/parallel/test-http-url.parse-post.js Outdated Show resolved Hide resolved
Copy link

codecov bot commented Nov 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.95%. Comparing base (3511c5c) to head (b50cbcd).
Report is 29 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #55996   +/-   ##
=======================================
  Coverage   87.95%   87.95%           
=======================================
  Files         656      656           
  Lines      188304   188376   +72     
  Branches    35972    35980    +8     
=======================================
+ Hits       165624   165691   +67     
+ Misses      15853    15850    -3     
- Partials     6827     6835    +8     
Files with missing lines Coverage Δ
lib/v8.js 98.65% <100.00%> (+0.01%) ⬆️

... and 32 files with indirect coverage changes

test/parallel/test-v8-serdes.js Outdated Show resolved Hide resolved
@@ -306,6 +307,9 @@ function arrayBufferViewIndexToType(index) {
if (index === 10) return FastBuffer;
if (index === 11) return BigInt64Array;
if (index === 12) return BigUint64Array;
// TODO(bartlomieju): use a primordial, once `Float16Array` is available in stable V8
// eslint-disable-line no-undef
if (index === 13) return Float16Array;
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't using undefined reference throw if there's actual [object Float16Array] polyfilled in user space?

const { serializeError, deserializeError } = require('internal/error_serdes');
Object.defineProperty(Float32Array.prototype, Symbol.toStringTag, { get() { return 'Float16Array'; } });
console.log(deserializeError(serializeError(new Error('test', { cause: new Float32Array(9) }))));
Suggested change
if (index === 13) return Float16Array;
if (index === 13) return typeof Float16Array !== 'undefined' ? Float16Array : undefined;

Copy link
Member

Choose a reason for hiding this comment

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

if there's actual [object Float16Array] polyfilled in user space?

I don't think that's much of a concern because you can't reasonably polyfill it in JS land. Speaking as someone who implemented Float16Array in a JS engine.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should destructure it from globalThis – it would be good to add a lint rule for it similar to

// ShadowRealm is not available in primordials because it can be
// disabled with --no-harmony-shadow-realm CLI flag.
{
name: 'ShadowRealm',
message: 'Use `const { ShadowRealm } = globalThis;` instead of the global.',
},

Copy link
Contributor

Choose a reason for hiding this comment

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

@bartlomieju wdyt about my suggestion?

@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 29, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 29, 2024
@nodejs-github-bot
Copy link
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Float16Array not working in node:v8 serde
7 participants