lib: convert transfer sequence to array in js

[jazelly]

This PR utilizes the webidl sequence converter to convert transfer in JS layer. Although this is kind of like undoing some efforts made to move the entire strucutredClone to C++, it fixes the inconsistency of structuredClone with other runtimes with webidl compliance.

Additionally, this reduces the C++ to JS overhead, as the iterator function is no longer invoked at C++ layer, when transfer is an iterator object.

Fixes: #55280
Refs: #50330

[nodejs-github-bot]

Review requested:

• @nodejs/startup
• @nodejs/web-standards

[aduh95]

Tests are failing:

TypeError: webidl.converters.sequence<transfer> is not a function
    at structuredClone (node:internal/structured_clone:35:62)

[codecov]

Codecov Report

Attention: Patch coverage is 86.36364% with 9 lines in your changes missing coverage. Please review.

Project coverage is 88.40%. Comparing base (8dbca2d) to head (c9f5904).
Report is 40 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/webidl.js	50.00%	6 Missing ⚠️
src/node_messaging.cc	70.00%	1 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #55317      +/-   ##
==========================================
- Coverage   88.41%   88.40%   -0.02%     
==========================================
  Files         652      652              
  Lines      186759   186827      +68     
  Branches    36097    36048      -49     
==========================================
+ Hits       165130   165165      +35     
- Misses      14898    14911      +13     
- Partials     6731     6751      +20     

Files with missing lines	Coverage Δ
...internal/bootstrap/web/exposed-window-or-worker.js	93.70% <100.00%> (+0.15%)	⬆️
lib/internal/perf/usertiming.js	94.14% <100.00%> (ø)
lib/internal/webstreams/readablestream.js	98.32% <100.00%> (-0.01%)	⬇️
lib/internal/worker/js_transferable.js	98.54% <100.00%> (+0.54%)	⬆️
src/node_messaging.cc	83.56% <70.00%> (-0.50%)	⬇️
lib/internal/webidl.js	95.04% <50.00%> (-1.74%)	⬇️

... and 58 files with indirect coverage changes

[jazelly]

Should be fixed now. Thanks for the heads up.

[jazelly]

cc @joyeecheung This is more like a RFC right now. I'm keen to hear your thoughts on this approach since you initiated the work moving structuredClone to C++.

[joyeecheung]

The work wasn't really started to move it entirely to C++, it was about getting rid of the unnecessary MessageChannel + MessagePort abstraction and doing the Message::Serialize/Message::Deserialize directly, which you couldn't do with what was exposed to JS. The most essential part was this

node/src/node_messaging.cc

Lines 1609 to 1613 in b5fb2ff

	if (msg->Serialize(env, context, value, transfer_list, Local<Object>())
	.IsNothing() ||
	!msg->Deserialize(env, context, nullptr).ToLocal(&result)) {
	return;
	}

And this PR is not touching it. The argument conversion should've been done in JS in the first place IMO, just not in the middle of binding, but should be done before the binding is invoked. In fact I actually left a TODO comment in there:

node/src/node_messaging.cc

Lines 1600 to 1601 in b5fb2ff

	// TODO(joyeecheung): implement this in JS land to avoid the C++ -> JS
	// cost to convert a sequence into an array.

Which I think is exactly what this PR is doing?

[jazelly]

Yes, exactly what this PR is trying to achieve : )

[joyeecheung]

LGTM with a nit

[mcollina]

lgtm

[nodejs-github-bot]

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

[KhafraDev]

I'm not going to block this PR, especially with a green CI, but having an object converter is not correct here. As noted in the PR, a dictionary converter is correct - as an idl dictionary can be represented in js, whilst an idl object cannot.

There are some small issues like hitting object.transfer's getter twice, which is observable via a proxy, and checking the type of options multiple times, but nothing major.

[nodejs-github-bot]

Landed in 50b4ada