Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stream: fix web streams have no Symbol.toStringTag #45117

Merged

Conversation

MrJithil
Copy link
Member

fix web streams have no Symbol.toStringTag

Issue: #45114

@benjamingr
Copy link
Member

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

@targos
Copy link
Member

targos commented Oct 22, 2022

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

@MrJithil
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Contributor

aduh95 commented Oct 22, 2022

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
Copy link
Member Author

MrJithil commented Oct 23, 2022

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.

@MrJithil MrJithil force-pushed the fix-web-streams-no-toStringTag branch from c1cb095 to 8aad2ba Compare October 23, 2022 03:20
@aduh95
Copy link
Contributor

aduh95 commented Oct 23, 2022

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
Copy link
Member Author

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
Copy link
Contributor

aduh95 commented Oct 23, 2022

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
Copy link
Member Author

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
Copy link
Contributor

aduh95 commented Oct 23, 2022

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
Copy link
Member

daeyeon commented Oct 24, 2022

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Contributor

aduh95 commented Oct 24, 2022

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
Copy link
Member Author

MrJithil commented Oct 24, 2022

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.

@MrJithil MrJithil force-pushed the fix-web-streams-no-toStringTag branch 2 times, most recently from dbf2c9a to 6f86553 Compare October 25, 2022 08:36
@benjamingr
Copy link
Member

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
Copy link
Member Author

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.

@MrJithil MrJithil force-pushed the fix-web-streams-no-toStringTag branch 4 times, most recently from eda0c87 to 1b8e812 Compare October 25, 2022 09:33
lib/internal/webstreams/readablestream.js Outdated Show resolved Hide resolved
lib/internal/webstreams/queuingstrategies.js Outdated Show resolved Hide resolved
lib/internal/webstreams/queuingstrategies.js Outdated Show resolved Hide resolved
lib/internal/webstreams/readablestream.js Outdated Show resolved Hide resolved
lib/internal/webstreams/readablestream.js Outdated Show resolved Hide resolved
lib/internal/webstreams/transformstream.js Outdated Show resolved Hide resolved
lib/internal/webstreams/writablestream.js Outdated Show resolved Hide resolved
lib/internal/webstreams/writablestream.js Outdated Show resolved Hide resolved
lib/internal/webstreams/writablestream.js Outdated Show resolved Hide resolved
lib/internal/webstreams/writablestream.js Outdated Show resolved Hide resolved
@MrJithil MrJithil force-pushed the fix-web-streams-no-toStringTag branch from 4f28416 to a1f6faf Compare October 25, 2022 11:48
stream: fix web streams have no Symbol.toStringTag

stream: add unit tests
@MrJithil MrJithil force-pushed the fix-web-streams-no-toStringTag branch from a1f6faf to 1e4a210 Compare October 25, 2022 11:50
@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 25, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 25, 2022
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. dont-land-on-v14.x labels Oct 25, 2022
@benjamingr benjamingr added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 26, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 26, 2022
@nodejs-github-bot nodejs-github-bot merged commit e9ba08e into nodejs:main Oct 26, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in e9ba08e

@benjamingr
Copy link
Member

Thanks @MrJithil !

@MrJithil MrJithil deleted the fix-web-streams-no-toStringTag branch October 26, 2022 22:42
RafaelGSS pushed a commit that referenced this pull request Nov 1, 2022
stream: fix web streams have no Symbol.toStringTag

stream: add unit tests
PR-URL: #45117
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Nov 1, 2022
RafaelGSS pushed a commit that referenced this pull request Nov 10, 2022
stream: fix web streams have no Symbol.toStringTag

stream: add unit tests
PR-URL: #45117
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
stream: fix web streams have no Symbol.toStringTag

stream: add unit tests
PR-URL: #45117
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
stream: fix web streams have no Symbol.toStringTag

stream: add unit tests
PR-URL: #45117
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
stream: fix web streams have no Symbol.toStringTag

stream: add unit tests
PR-URL: #45117
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
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. needs-ci PRs that need a full CI run. web streams
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants