Skip to content

Conversation

@IlyasShabi
Copy link
Contributor

This PR aims to add support for Float16Array in n-api, addressing this issue
cc: @nodejs/node-api

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/node-api

@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. node-api Issues and PRs related to the Node-API. labels Jun 28, 2025
@IlyasShabi IlyasShabi changed the title napi: add support for Float16Array node-api: add support for Float16Array Jun 28, 2025
@IlyasShabi IlyasShabi force-pushed the ishabi/napi-float16array branch from 48a7303 to 3d5bf56 Compare June 28, 2025 22:54
@codecov
Copy link

codecov bot commented Jun 28, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.56%. Comparing base (bdf03bf) to head (63f957e).
⚠️ Report is 169 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #58879      +/-   ##
==========================================
+ Coverage   88.05%   88.56%   +0.50%     
==========================================
  Files         704      704              
  Lines      207857   207861       +4     
  Branches    39964    40049      +85     
==========================================
+ Hits       183030   184092    +1062     
+ Misses      16806    15802    -1004     
+ Partials     8021     7967      -54     
Files with missing lines Coverage Δ
src/js_native_api_v8.cc 76.59% <100.00%> (+0.04%) ⬆️

... and 101 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.

@IlyasShabi IlyasShabi requested a review from addaleax June 29, 2025 11:26
jasnell
jasnell previously approved these changes Jun 29, 2025
Copy link
Member

@vmoroz vmoroz left a comment

Choose a reason for hiding this comment

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

We need to ensure that the new Node-API changes follow the Node-API change requirements outlined in this document: https://github.com/nodejs/node/blob/main/doc/contributing/adding-new-napi-api.md

Note that the requirement that says "Should be implemented in at least one other VM implementation of Node.js." - it is currently being discussed to be revised towards something like "being implementable by other JS runtimes and JS engines" - see #58852. I.e. the new API like in this PR must be good as long as it is not V8-specific.

@jasnell jasnell dismissed their stale review June 29, 2025 20:41

ABI breaking changes made in subsequent commits. Removing the approval until all issues can be resolved

@jasnell
Copy link
Member

jasnell commented Jun 29, 2025

@IlyasShabi ... to address the ABI breaking issues, you might take some inspiration from this duplicate PR #58889

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Feels quite silly to put this behind an experimental flag – this seems more of a "fix" to account for the fact that the language now exposes a new variant, rather than a new feature.

@legendecas legendecas moved this from Need Triage to In Progress in Node-API Team Project Jul 4, 2025
Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

Discussed on node-api meeting, for this PR to be able to be backported to previous LTS lines, we will need a new napi_status as napi_not_supported, so that on Node.js v22 and v20, the api napi_create_typedarray return napi_not_supported if napi_float16_array is requested, even if the addon is compiled with NAPI_EXPERIMENTAL, or newer napi version.

And for the enum value ABI stability, we will have discussions in #59085 resolved and unblock this PR.

@vmoroz vmoroz self-requested a review August 1, 2025 14:21
@IlyasShabi IlyasShabi marked this pull request as ready for review October 30, 2025 22:17
@IlyasShabi IlyasShabi requested a review from KevinEady October 30, 2025 22:18
@IlyasShabi IlyasShabi force-pushed the ishabi/napi-float16array branch from fcec785 to 63f957e Compare October 31, 2025 07:55
Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

Thank you

@legendecas legendecas added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Nov 11, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 11, 2025
@nodejs-github-bot
Copy link
Collaborator

@legendecas
Copy link
Member

@KevinEady @vmoroz would you mind taking a look at this updated PR again? It is necessary to address the change requests before this can be landed. Thank you!

@legendecas legendecas 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 Nov 18, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 18, 2025
@nodejs-github-bot nodejs-github-bot merged commit 900d329 into nodejs:main Nov 18, 2025
74 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 900d329

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API.

Projects

Development

Successfully merging this pull request may close these issues.

8 participants