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

util: fix TypeError when console.log a static property name in class #42790

Closed
wants to merge 2 commits into from
Closed

util: fix TypeError when console.log a static property name in class #42790

wants to merge 2 commits into from
+6 −1

Conversation

cola119
Copy link
Member

@cola119 cola119 commented Apr 20, 2022 •
edited
Loading

Fixed #42773

Constructor name from getConstructorName will be converted to string by template literal (${constructor}) in a subsequent process.

In this case, getConstructorName need to convert symbol to string by Symbol.prototype.toString beforehand since ${Symbol()} throws TypeError (spec).

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels Apr 20, 2022
@meixg meixg added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 20, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 20, 2022
@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/43579/

This was referenced Apr 21, 2022
Open
Open
BridgeAR
BridgeAR reviewed Apr 22, 2022
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is definitely the right spot where to fix it!

Ti improve it further, we could just use String(). That way other types are also handled properly.

lib/internal/util/inspect.js Outdated
Comment on lines 582 to 583
const name = descriptor.value.name;
return typeof name === 'symbol' ? SymbolPrototypeToString(name) : name;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const name = descriptor.value.name;
return typeof name === 'symbol' ? SymbolPrototypeToString(name) : name;
return String(descriptor.value.name);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. That makes sense!

@github-actions github-actions bot mentioned this pull request Apr 23, 2022
Open
21 tasks
@cola119 cola119 requested a review from BridgeAR April 23, 2022 03:52
This was referenced Apr 24, 2022
Open
Open
@watilde watilde added the request-ci Add this label to start a Jenkins CI on a PR. label May 19, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 19, 2022
@nodejs-github-bot
Copy link
Collaborator

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

This was referenced May 20, 2022
Open
Open
Open
Open
Open
Open
Open
@cola119
Copy link
Member Author

cola119 commented Jun 5, 2022

@jasnell @BridgeAR Could you please re-trigger the test and merge? Thank you in advance!

@BridgeAR BridgeAR added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Jun 11, 2022
@panva panva added request-ci Add this label to start a Jenkins CI on a PR. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jun 16, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 16, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Jun 17, 2022
Open
31 tasks
@nodejs-github-bot
Copy link
Collaborator

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

@panva panva closed this Jun 17, 2022
panva pushed a commit that referenced this pull request Jun 17, 2022
@cola119 @panva
util: fix TypeError of symbol in template literals
027c288 
PR-URL: #42790
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@panva
Copy link
Member

panva commented Jun 17, 2022

Landed in 027c288

This was referenced Jun 18, 2022
Open
Open
targos pushed a commit that referenced this pull request Jul 12, 2022
@cola119 @targos
util: fix TypeError of symbol in template literals
a1f581a 
PR-URL: #42790
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@targos targos mentioned this pull request Jul 12, 2022
Merged
targos pushed a commit that referenced this pull request Jul 18, 2022
@cola119 @targos
util: fix TypeError of symbol in template literals
26e706f 
PR-URL: #42790
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
targos pushed a commit that referenced this pull request Jul 31, 2022
@cola119 @targos
util: fix TypeError of symbol in template literals
3959dff 
PR-URL: #42790
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@targos targos mentioned this pull request Aug 3, 2022
Merged
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
@cola119 @guangwong
util: fix TypeError of symbol in template literals
0ad3635 
PR-URL: nodejs/node#42790
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
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. util Issues and PRs related to the built-in util module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

TypeError when console.log a static property name in class
7 participants
@cola119 @nodejs-github-bot @panva @jasnell @BridgeAR @watilde @meixg