nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
104.27k stars 28.06k forks source link

Define node::StreamBase's "onread" property as a JavaScript accessor #52758

Open isheludko opened 2 weeks ago

isheludko commented 2 weeks ago

Version

v22.0.0-pre

Platform

No response

Subsystem

No response

What steps will reproduce the bug?

V8 is deprecating v8::ObjectTemplate::SetAccessor(..) (see https://crbug.com/336325111) which is used by node::StreamBase for defining "onread" property. V8 suggests to use v8::ObjectTemplate::SetNativeDataProperty(..) instead but it has a slightly different behaviour and Node is currently relying on the old one.

The SetAccessor(..) defines a JavaScript data property which violates the JavaScript spec for the following use case:

  var proto = stream;
  var obj = Object.create(proto);
  obj.onread = 42; // [*]

According to the spec [*] should set the onread property on obj while for a SetAccessor-defined property V8 would trigger the property setter callback. The SetNativeDataProperty-defined property would behave like a regular data property according to the spec.

Node defines "onread" property on the stream's prototype object while it sets the value via stream instance objects. This approach heavily depends on non-standard SetAccessor's semantics. The right fix would be to define "onread" property as a JavaScript accessor using v8::ObjectTemplate::SetAccessorProperty(..).

The following fix for Node-ci seems to work: https://github.com/v8/node/pull/183

How often does it reproduce? Is there a required condition?

No response

What is the expected behavior? Why is that the expected behavior?

No response

What do you see instead?

n/a

Additional information

No response