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.

Sign up for GitHub

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

Jump to bottom

perf_hooks: align @@toStringTag with other Web Performance implementations #45157

Merged
merged 3 commits into from
Oct 27, 2022
Merged

perf_hooks: align @@toStringTag with other Web Performance implementations #45157

merged 3 commits into from
Oct 27, 2022

Conversation

daeyeon
Copy link
Member

@daeyeon daeyeon commented Oct 25, 2022

This gets Symbol.toStringTag on Web Performance APIs to be aligned
with the other runtime implementations. (Tested on Chromium, Firefox, and Safari)

Signed-off-by: Daeyeon Jeong [email protected]

@daeyeon
perf_hooks: align toStringTag with other Web Performance implementations
f2cb008 
This gets `Symbol.toStringTag` on Web Performance APIs to be aligned
with the other runtime implementations.

Signed-off-by: Daeyeon Jeong <[email protected]>
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Oct 25, 2022
@daeyeon daeyeon added the perf_hooks Issues and PRs related to the implementation of the Performance Timing API. label Oct 25, 2022
@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Oct 25, 2022
@JithilNCA
Copy link

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/get#get_vs._defineproperty

@daeyeon
Copy link
Member Author

daeyeon commented Oct 25, 2022

Can we refer to https://webidl.spec.whatwg.org/#ecmascript-binding?

If an object has a class string classString, then the object must, at the time it is created, have a property whose name is the @@toStringTag symbol with PropertyDescriptor{[[Writable]]: false, [[Enumerable]]: false, [[Configurable]]: true, [[Value]]: classString}.

@MrJithil
Copy link
Member

Could you please explain, whats this code changes will helps to do?

The get is syntax which will convert exactly same thing you defined using configuration.

Then, why we need to modify the working code?

If you think, this is a standard, i recommend to make a coding standard guidlines first.

Please have a look here https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/get

@MrJithil
Copy link
Member

Screen Shot 2022-10-25 at 1 49 03 pm

@MrJithil
Copy link
Member

If you are really thinks this is a standard, worthy, and we must follow that, please create a ticket.

I can convert another instance of getters and move to define props including my open PR #45114

@daeyeon
Copy link
Member Author

daeyeon commented Oct 25, 2022

Yes, I think that we can meet the specification more with this way. Please take a look at the WPTs below.

https://github.com/web-platform-tests/wpt/blob/a370aad338d6ed743abb4d2c6ae84a7f1058558c/webidl/ecmascript-binding/class-string-interface.any.js#L3-L11

https://github.com/web-platform-tests/wpt/search?l=JavaScript&q=toStringTag

@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

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@MrJithil
Copy link
Member

MrJithil commented Oct 25, 2022 •
edited
Loading

Make sense now. This could be a valid reason to use Define property instead of getter. Just to align with wpt. Nothing else.

Making changes accordingly.

@daeyeon
fixup: apply the review
a2cd0ae 
Signed-off-by: Daeyeon Jeong <[email protected]>
@daeyeon daeyeon added commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. request-ci Add this label to start a Jenkins CI on a PR. labels Oct 25, 2022
@aduh95 aduh95 removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 25, 2022
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@daeyeon
fixup: apply the review
07b8444 
Signed-off-by: Daeyeon Jeong <[email protected]>
@github-actions github-actions bot mentioned this pull request Oct 26, 2022
Open
14 tasks
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Oct 27, 2022
Open
13 tasks
@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 27, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 27, 2022
@nodejs-github-bot nodejs-github-bot merged commit 631bdb4 into nodejs:main Oct 27, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 631bdb4

This was referenced Oct 28, 2022
Open
Open
Open
Open
RafaelGSS pushed a commit that referenced this pull request Nov 1, 2022
@daeyeon @RafaelGSS
perf_hooks: align toStringTag with other Web Performance implementations
8a5f6d3 
This gets `Symbol.toStringTag` on Web Performance APIs to be aligned
with the other runtime implementations.

Signed-off-by: Daeyeon Jeong <[email protected]>
PR-URL: #45157
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Feng Yu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Nov 1, 2022
Merged
RafaelGSS pushed a commit that referenced this pull request Nov 10, 2022
@daeyeon @RafaelGSS
perf_hooks: align toStringTag with other Web Performance implementations
dfe4237 
This gets `Symbol.toStringTag` on Web Performance APIs to be aligned
with the other runtime implementations.

Signed-off-by: Daeyeon Jeong <[email protected]>
PR-URL: #45157
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Feng Yu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@danielleadams
Copy link
Contributor

@daeyeon this doesn't land cleanly on v18.x. Do you mind making a backport PR?

@danielleadams danielleadams added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Dec 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MrJithil MrJithil MrJithil left review comments

@aduh95 aduh95 aduh95 approved these changes

@F3n67u F3n67u F3n67u approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. perf_hooks Issues and PRs related to the implementation of the Performance Timing API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@daeyeon @JithilNCA @MrJithil @nodejs-github-bot @danielleadams @jasnell @F3n67u @aduh95