lib: implement webidl dictionary converter and use it in structuredClone

[jazelly]

The first commit optimizes type(V) in webidl.js to return enum
instead of string.

The second commit provides a factory to generate dictionaryConverter
compliant with the spec. The implemented factory function is used for
the structuredClone algorithm, along with updated test cases.

[jazelly]

This is not the first time gh pr create --fill gives unexpected PR title and messages. -_-

[codecov]

Codecov Report

Attention: Patch coverage is 86.40777% with 14 lines in your changes missing coverage. Please review.

Project coverage is 88.43%. Comparing base (7b5d660) to head (e72a0c7).
Report is 54 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/webidl.js	83.33%	14 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #55489      +/-   ##
==========================================
+ Coverage   88.41%   88.43%   +0.01%     
==========================================
  Files         653      654       +1     
  Lines      187476   187735     +259     
  Branches    36083    36125      +42     
==========================================
+ Hits       165763   166019     +256     
- Misses      14946    14954       +8     
+ Partials     6767     6762       -5     

Files with missing lines	Coverage Δ
lib/internal/worker/js_transferable.js	98.55% <100.00%> (+0.01%)	⬆️
lib/internal/webidl.js	93.16% <83.33%> (-1.89%)	⬇️

... and 43 files with indirect coverage changes

[KhafraDev]

LGTM, some small nits

[KhafraDev]

@anonrig I am not sure why this needs a benchmark CI.

[anonrig]

@anonrig I am not sure why this needs a benchmark CI.

Because it impacts structedClone. It's good to know the impacts of such highly used function.

[KhafraDev]

I'm not sure I agree because benchmark CIs are typically used for performance-related PRs. I can't see a reason to have one here other than to block the change. This fixes a number of small bugs (I outlined these issues in the PR that started to use the webidl sequence converter), this is not a PR related to performance.

Likewise, the more steps we take away from webidl, the less valuable it becomes. It's fine to compare a few strings now, and change it to an enum-like value in the future IMO.

[jazelly]

benchmark: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1649/

result:

                                                    confidence improvement accuracy (*)   (**)  (***)
misc/structured-clone.js n=10000 type='arraybuffer'        ***    -13.20 %       ±2.37% ±3.16% ±4.11%
misc/structured-clone.js n=10000 type='object'               *      0.46 %       ±0.42% ±0.55% ±0.72%
misc/structured-clone.js n=10000 type='string'              **      3.06 %       ±2.30% ±3.06% ±3.98%

[anonrig]

cc @nodejs/performance

[mcollina]

lgtm

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/63283/ 💛

[anonrig]

I'm not blocking but is 13% performance degregation worth this change? cc @mcollina

[KhafraDev]

I'm not blocking but is 13% performance degregation worth this change?

node isn't properly implementing structuredClone without this change. The baseline should be this PR.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/63289/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/63291/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/63306/

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/55489
✔  Done loading data for nodejs/node/pull/55489
----------------------------------- PR info ------------------------------------
Title      lib: implement webidl dictionary converter and use it in structuredClone (#55489)
Author     Jason Zhang <[email protected]> (@jazelly)
Branch     jazelly:dictionary-converter -> nodejs:main
Labels     author ready, worker, web-standards
Commits    2
 - lib: prefer number than string in webidl `type` function
 - lib: implement webidl dictionary converter and use it in structuredClone
Committers 1
 - Jason Zhang <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/55489
Reviewed-By: Matthew Aitken <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/55489
Reviewed-By: Matthew Aitken <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - lib: implement webidl dictionary converter and use it in structuredClone
   ℹ  This PR was created on Tue, 22 Oct 2024 11:19:21 GMT
   ✔  Approvals: 3
   ✔  - Matthew Aitken (@KhafraDev): https://github.com/nodejs/node/pull/55489#pullrequestreview-2385375542
   ✔  - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/55489#pullrequestreview-2392490011
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/55489#pullrequestreview-2395332839
   ✔  Last GitHub CI successful
   ℹ  Last Benchmark CI on 2024-10-25T10:57:25Z: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1649/
   ℹ  Last Full PR CI on 2024-10-27T02:31:21Z: https://ci.nodejs.org/job/node-test-pull-request/63306/
- Querying data for job/node-test-pull-request/63306/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/11643709674

[benjamingr]

Not blocking either but like @anonrig I'm -0 on this

approved by mistake, meant to comment