FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

perf_hooks: align `@@toStringTag` with other Web Performance implementations by daeyeon · Pull Request #45157 · nodejs/node · GitHub

/ node Public

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

Merged
nodejs-github-bot merged 3 commits into
nodejs:mainfrom
daeyeon:main.test-@@tostringtag-221024.Mon.f3c0
Oct 27, 2022
ConversationCommits (3)ChecksFiles changed
Merged

perf_hooks: align @@toStringTag with other Web Performance implementations#45157
nodejs-github-bot merged 3 commits into
nodejs:mainfrom
daeyeon:main.test-@@tostringtag-221024.Mon.f3c0

Conversation

daeyeon commented Oct 25, 2022

Copy link
Copy Markdown
Member

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 daeyeon.dev@gmail.com

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 <daeyeon.dev@gmail.com>
nodejs-github-bot added the needs-ci PRs that need a full CI run. label Oct 25, 2022
daeyeon added the perf_hooks Issues and PRs related to the implementation of the Performance Timing API. label Oct 25, 2022
aduh95 added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Oct 25, 2022

JithilNCA commented Oct 25, 2022

Copy link
Copy Markdown

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

daeyeon commented Oct 25, 2022

Copy link
Copy Markdown
Member Author

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 commented Oct 25, 2022

Copy link
Copy Markdown
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 commented Oct 25, 2022

Copy link
Copy Markdown
Member

MrJithil commented Oct 25, 2022

Copy link
Copy Markdown
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 commented Oct 25, 2022

Copy link
Copy Markdown
Member Author

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 Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 25, 2022

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view

MrJithil commented Oct 25, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

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.

MrJithil reviewed Oct 25, 2022
View reviewed changes
Comment thread test/parallel/test-perf-hooks-usertiming.js Outdated
fixup: apply the review
a2cd0ae 
Signed-off-by: Daeyeon Jeong <daeyeon.dev@gmail.com>
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 reviewed Oct 25, 2022
View reviewed changes
Comment thread test/parallel/test-perf-hooks-usertiming.js Outdated
aduh95 removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 25, 2022

This comment was marked as outdated.

Sign in to view
fixup: apply the review
07b8444 
Signed-off-by: Daeyeon Jeong <daeyeon.dev@gmail.com>
github-actions Bot mentioned this pull request Oct 26, 2022
Open
14 tasks

This comment was marked as outdated.

Sign in to view

nodejs-github-bot commented Oct 26, 2022

Copy link
Copy Markdown
Collaborator

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

github-actions Bot mentioned this pull request Oct 27, 2022
Open
13 tasks
aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 27, 2022
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 merged commit 631bdb4 into nodejs:main Oct 27, 2022

nodejs-github-bot commented Oct 27, 2022

Copy link
Copy Markdown
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
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 <daeyeon.dev@gmail.com>
PR-URL: #45157
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Feng Yu <F3n67u@outlook.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
RafaelGSS mentioned this pull request Nov 1, 2022
Merged
RafaelGSS pushed a commit that referenced this pull request Nov 10, 2022
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 <daeyeon.dev@gmail.com>
PR-URL: #45157
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Feng Yu <F3n67u@outlook.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

danielleadams commented Dec 13, 2022

Copy link
Copy Markdown
Contributor

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

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

aduh95 aduh95 approved these changes
MrJithil MrJithil left review comments
jasnell jasnell approved these changes
+1 more reviewer
F3n67u F3n67u approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. 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

Back | FazBrowse Home | New Git URL