stream: fix web streams have no Symbol.toStringTag

[MrJithil]

fix web streams have no Symbol.toStringTag

Issue: #45114

[benjamingr]

Looks like there are some relevant build failures that are pretty easy to address :]

[targos]

I don't understand how switching to a getter fixes the issue.

[MrJithil]

I don't understand how switching to a getter fixes the issue.

Actually, we missed the getter implementation before. It should be a getter, to return some results.

[MrJithil]

Looks like there are some relevant build failures that are pretty easy to address :]

I will check it out and fix next day. Its seems like missing of setters. But, we don't need setters anymore. I will fix them.

[aduh95]

I don't understand how switching to a getter fixes the issue.

Here's what I understand: when using a getter, it sets a kType property on the prototype, without the getter, nothing gets added to the prototype, only to the instance when call the constructor.
When checking TransformStream.prototype[Symbol.toStringTag], it returns the value of this[kType], and this is TransformStream.prototype in this case, not an instance of the class.

I don't understand how switching to a getter fixes the issue.

Actually, we missed the getter implementation before. It should be a getter, to return some results.

I disagree, I think we should not be using getters at all, none of the other implementations (I tried Firefox, Safari, Chromium, and Deno) use a getter for @@toStringTag:

> console.log(Object.getOwnPropertyDescriptor(TransformStream.prototype, Symbol.toStringTag))
Object { value: "TransformStream", writable: false, enumerable: false, configurable: true }

IMHO a better fix would be (I'm using TransformStream here as an example, the same applies to the other classes):

diff --git a/lib/internal/webstreams/transformstream.js b/lib/internal/webstreams/transformstream.js
index b7366fb1ba..114d4d4e16 100644
--- a/lib/internal/webstreams/transformstream.js
+++ b/lib/internal/webstreams/transformstream.js
@@ -102,8 +102,6 @@ const assert = require('internal/assert');
 class TransformStream {
   [kType] = 'TransformStream';
 
-  get [SymbolToStringTag]() { return this[kType]; }
-
   /**
    * @param {Transformer} [transformer]
    * @param {QueuingStrategy} [writableStrategy]
@@ -236,6 +234,11 @@ class TransformStream {
 ObjectDefineProperties(TransformStream.prototype, {
   readable: kEnumerableProperty,
   writable: kEnumerableProperty,
+  [SymbolToStringTag]: {
+    __proto__: null,
+    value: 'TransformStream',
+    configurable: true,
+  }
 });

[MrJithil]

I don't understand how switching to a getter fixes the issue.

I think that's a [[Define]] vs [[Set]] issue, when using a getter, it sets a kType property on the prototype, without the getter, nothing gets added to the prototype, only to the instance when call the constructor. When checking TransformStream.prototype[Symbol.toStringTag], it returns this[kType], and this is TransformStream.prototype in this case, not an instance of the class.

I don't understand how switching to a getter fixes the issue.

Actually, we missed the getter implementation before. It should be a getter, to return some results.

I disagree, I think we should not be using getters at all, none of the other implementations (I tried Firefox, Safari, Chromium, and Deno) use a getter for @@toStringTag:

> console.log(Object.getOwnPropertyDescriptor(TransformStream.prototype, Symbol.toStringTag))
Object { value: "TransformStream", writable: false, enumerable: false, configurable: true }

IMHO a better fix would be (I'm using TransformStream here as an example, the same applies to the other classes):

diff --git a/lib/internal/webstreams/transformstream.js b/lib/internal/webstreams/transformstream.js
index b7366fb1ba..114d4d4e16 100644
--- a/lib/internal/webstreams/transformstream.js
+++ b/lib/internal/webstreams/transformstream.js
@@ -102,8 +102,6 @@ const assert = require('internal/assert');
 class TransformStream {
   [kType] = 'TransformStream';
 
-  get [SymbolToStringTag]() { return this[kType]; }
-
   /**
    * @param {Transformer} [transformer]
    * @param {QueuingStrategy} [writableStrategy]
@@ -236,6 +234,11 @@ class TransformStream {
 ObjectDefineProperties(TransformStream.prototype, {
   readable: kEnumerableProperty,
   writable: kEnumerableProperty,
+  [SymbolToStringTag]: {
+    __proto__: null,
+    value: 'TransformStream',
+    configurable: true,
+  }
 });

I would like to keep the getter here, while comparing the other implementation areas of SymbolToStringTag , this property should not be writable. It is a readonly property and so we don't need to configure it as configurable.

In short, there are many places we sets the property because while reading the SymbolToStringTag returns undefined as the getter are not set.

[aduh95]

I would like to keep the getter here, while comparing the other implementation areas of SymbolToStringTag , this property should not be writable. It is a readonly property and so we don't need to configure it as configurable.

This doesn't have anything to do with using a getter or using a value, right?

In short, there are many places we sets the property because while reading the SymbolToStringTag returns undefined as the getter are not set.

Yeah, that's why I was saying if we are no longer using getters, it fixes the issue AND it aligns with the other implementations. I feel quite strongly that getters are not the way to go here.

[MrJithil]

I would like to keep the getter here, while comparing the other implementation areas of SymbolToStringTag , this property should not be writable. It is a readonly property and so we don't need to configure it as configurable.

This doesn't have anything to do with using a getter or using a value, right?

In short, there are many places we sets the property because while reading the SymbolToStringTag returns undefined as the getter are not set.

Yeah, that's why I was saying if we are no longer using getters, it fixes the issue AND it aligns with the other implementations. I feel quite strongly that getters are not the way to go here.

We are using getters . But, no need of any setters

[aduh95]

We are using getters . But, no need of any setters

I'm not asking for any setters, I'm asking to get rid of getters. If you look at the diff in #45117 (comment), hopefully you can see what I mean.

[MrJithil]

We are using getters . But, no need of any setters

I'm not asking for any setters, I'm asking to get rid of getters. If you look at the diff in #45117 (comment), hopefully you can see what I mean.

What's the reason for your suggestion of not using getters? I am not intending to add some SymbolToStringTag . Instead, I want fix the kType not readable issue. Feel free to make the changes on top of it.

In this PR, I want to minimise the changes.

[aduh95]

What's the reason for your suggestion of not using getters?

Aligning with the other implementations should be enough of a reason. But also, by using a getter the [kType] property would no longer be added to each instance which doesn't seem like a good design – at this point, why bother with kType and not use .constructor.name.

In this PR, I want to minimise the changes.

If you allow me to return you the question: what's the reason for your suggestion of minimising the changes? IMO producing the correct fix is more important than having a small diff, and arguably my suggestion wouldn't increase the magnitude of the diff.

Feel free to make the changes on top of it.

I've opened #45136 as an alternative to this PR.

[daeyeon]

It seems common implementations for built-in properties whose keys are Symbol.toStringTag to be defined with data descriptors. And [[writable]]: false looks clearer than an empty setter.

# console.log(Object.getOwnPropertyDescriptor(ByteLengthQueuingStrategy.prototype, Symbol.toStringTag)

# data property
{
  value: 'ByteLengthQueuingStrategy',
  writable: false,
  enumerable: false,
  configurable: true
}

# accessor property
{
  get: [Function: get [Symbol.toStringTag]],
  set: undefined,
  enumerable: false,
  configurable: true
}

[MrJithil]

It seems common implementations for built-in properties whose keys are Symbol.toStringTag to be defined with data descriptors. And [[writable]]: false looks clearer than an empty setter.

# console.log(Object.getOwnPropertyDescriptor(ByteLengthQueuingStrategy.prototype, Symbol.toStringTag)



# data property

{

  value: 'ByteLengthQueuingStrategy',

  writable: false,

  enumerable: false,

  configurable: true

}



# accessor property

{

  get: [Function: get [Symbol.toStringTag]],

  set: undefined,

  enumerable: false,

  configurable: true

}

I am not trying to define toStringTag here.

I am fixing the undefined issue kType.

For that, I don't like to make it more complex by the above suggestions.

I want to proceed with the getter approach, and its looks more elegant to me.

Thanks your suggestion.

[MrJithil]

Also, I don't think its fair to discard someone’s pull request with an alternate PR.

I stand with my changes because there is no difference between define property and getter.

I dont want to override the implementation of toStringTag, instead I fixed the undefined kType, which is being used as a toStringTag. Also, i want to keep my changes very minimal.

Code of conduct doc may need revisits.

CC: @targos @aduh95

[aduh95]

Also, I don't think its fair to discard someone’s pull request with an alternate PR.

I'm sorry to hear that, I didn't want to discard your PR, I read "Feel free to make the changes on top of it" as an invitation to propose my changes separately. But to be fair, since you are not willing to take my suggestion, I don't think I had much choice but to open my own PR. What would you suggest me to do?

I stand with my changes because there is no difference between define property and getter.

That is simply not true, of course there are observable differences between a getter and a non-writable property.

I dont want to override the implementation of toStringTag, instead I fixed the undefined kType, which is being used as a toStringTag.

That's unfortunate, as IMO the @@toStringTag implementation deviating from the other runtimes is the root cause for the linked issue. It's fair if you want to stick with your changes, and I hope you understand if it's fair for me to disagree.

Code of conduct doc may need revisits.

We might need to discuss this is a separate issue/PR to keep this one on topic, but if we can improve the CoC I'm all for it.

[MrJithil]

Also, I don't think its fair to discard someone’s pull request with an alternate PR.

I'm sorry to hear that, I didn't want to discard your PR, I read "Feel free to make the changes on top of it" as an invitation to propose my changes separately. But to be fair, since you are not willing to take my suggestion, I don't think I had much choice but to open my own PR. What would you suggest me to do?

You can make your suggestion later as different PR or commit on this PR. But, I can not accept your changes because there are no difference in them.
Please refer here to learn the getter https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/get

I stand with my changes because there is no difference between define property and getter.

That is simply not true, of course there are observable differences between a getter and a non-writable property.

What's your achievement here? Are you saving execution time? By using the defineProperty, you are not fixing the root cause of the issue. The root cause is kType undefined. Its might being read many place, so I don't want to override the implementation.

Also, there is no difference between the properties defined using getter, and defineProperty. However, if there are some difference, how it will impact the code quality?

Many people over the world is contributing to this repository. Everyone will follow their own style of coding. For instance, someone will use ternary operator, someone will use if etc. There is nothing good or bad. If it is breaking the code quality, then we should not promote those changes, Here, its not violating the code quality. Its just a thought others applying here because they are not seeing the actual fix.

I dont want to override the implementation of toStringTag, instead I fixed the undefined kType, which is being used as a toStringTag toStringTag. Can you spot that on my PR? FYI, I am following the same patter as other properties in the class. There are many 20-25 instance in each file they using the get, Then what the problem?

No-one is making changes to

That's unfortunate, as IMO the @@toStringTag implementation deviating from the other runtimes is the root cause for the linked issue. It's fair if you want to stick with your changes, and I hope you understand if it's fair for me to disagree.

You can disagree. I ma not checking other browsers to see the implementation. Can you provide the code or screenshot of that implementation? For your info, a getter is being converted to something we are adding with a define prop later. That may be the reason you are seeing it. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/get#defining_a_getter_on_existing_objects_using_defineproperty

Code of conduct doc may need revisits.

We might need to discuss this is a separate issue/PR to keep this one on topic, but if we can improve the CoC I'm all for it.

Yes. People are using their privileges to discard others is not good practice.

Please don't think others chooses are not right. Also, I don't want to discuss further as you are not understanding the getter and define props doing the similar results. I don't like to repeat the same conversations again. Thank you.

[benjamingr]

What's your achievement here? Are you saving execution time? By using the defineProperty, you are not fixing the root cause of the issue. The root cause is kType undefined. Its might being read many place, so I don't want to override the implementation.

I think Antoine's point was that spec-wise this fits with a non-writable property and not a getter. There are several observable difference for example:

class Foo {
  get a() { return 5 }
}
Object.defineProperty(Foo.prototype, "b", {
  value: 6,
  configurable: true
});
const f = new Foo();
Object.getOwnPropertyDescriptor(f.__proto__, 'a');
// {set: undefined, enumerable: false, configurable: true, get: ƒ}
Object.getOwnPropertyDescriptor(f.__proto__, 'b');
// {value: 6, writable: false, enumerable: false, configurable: true}

[MrJithil]

What's your achievement here? Are you saving execution time? By using the defineProperty, you are not fixing the root cause of the issue. The root cause is kType undefined. Its might being read many place, so I don't want to override the implementation.

I think Antoine's point was that spec-wise this fits with a non-writable property and not a getter. There are several observable difference for example:

class Foo {
  get a() { return 5 }
}
Object.defineProperty(Foo.prototype, "b", {
  value: 6,
  configurable: true
});
const f = new Foo();
Object.getOwnPropertyDescriptor(f.__proto__, 'a');
// {set: undefined, enumerable: false, configurable: true, get: ƒ}
Object.getOwnPropertyDescriptor(f.__proto__, 'b');
// {value: 6, writable: false, enumerable: false, configurable: true}

The initial suggestion was not acceptable. But, once its suggested with the wpt, I made the changes accordingly.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in e9ba08e

[benjamingr]

Thanks @MrJithil !