Skip to content

util: if present, fallback to toString using the %s formatter #27621

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
Closed
wants to merge 2 commits into from
Closed

util: if present, fallback to toString using the %s formatter #27621

wants to merge 2 commits into from
+61 −16

Conversation

@BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented May 9, 2019

This makes sure that util.format uses String to stringify an object
in case the object has an own property named toString with type
function. That way objects that do not have such function are still
inspected using util.inspect and the old behavior is preserved as
well.

Refs: jestjs/jest#8443

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 9, 2019

Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3471

@nodejs-github-bot nodejs-github-bot added the util Issues and PRs related to the built-in util module. label May 9, 2019
@BridgeAR BridgeAR added the review wanted PRs that need reviews. label May 10, 2019
@BridgeAR
Copy link
Member Author

BridgeAR commented May 10, 2019

@nodejs/util PTAL

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 10, 2019

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

addaleax
addaleax reviewed May 10, 2019
View reviewed changes
Fishrock123
Fishrock123 reviewed May 10, 2019
View reviewed changes
Copy link
Contributor

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

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

Change seems reasonable to me otherwise, pending Anna's comments.

@BridgeAR BridgeAR requested review from Fishrock123 and addaleax May 12, 2019 20:19
@BridgeAR BridgeAR force-pushed the odd-to-string-behavior branch from 72e1b7e to 9700e97 Compare May 13, 2019 22:38
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 13, 2019

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

@BridgeAR
Copy link
Member Author

BridgeAR commented May 15, 2019

@nodejs/util PTAL

silverwind
silverwind approved these changes May 15, 2019
View reviewed changes
Copy link
Contributor

@silverwind silverwind left a comment

Choose a reason for hiding this comment

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

LGTM but can you explain why we don't move this check to util.inspect?

@BridgeAR
Copy link
Member Author

BridgeAR commented May 15, 2019

@silverwind that would be quite a big breaking change and I doubt that we actually profit from that. It reminds me of our original inspect property. This case is of course a bit different because it's from the language itself but util.inspect is supposed not to trigger any side effects while inspecting the object and it should return the "true" value in case our custom inspect function is not used. Otherwise the original custom inspection could have just been to check for toString.
Neither Chrome or Firefox do this either. They behave similar to our current util.inspect. In fact, Chrome actually does not even fall back to toString when using the %s formatter (Firefox does).

The reason why I opened this PR is to keep the behavior backwards compatible (using toString was the default before 12.x) and because the console spec defines this behavior (but the spec fell behind the actual Browser implementations and they often deviate from the spec and also from each other. We also deviate from it in cases where it seems useful, BigInt for example).

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 15, 2019
@BridgeAR
Copy link
Member Author

BridgeAR commented May 16, 2019

@nodejs/util PTAL. This could use another review.

BridgeAR added 2 commits May 16, 2019 15:40
@BridgeAR
doc: update util.format formatters documentation
6402452 
This includes the information that some inputs are handled
differently than others (e.g., `Symbols` are partially represented
as `NaN`).
@BridgeAR
util: if present, fallback to toString using the %s formatter
0bb8a7f 
This makes sure that `util.format` uses `String` to stringify an object
in case the object has an own property named `toString` with type
`function`. That way objects that do not have such function are still
inspected using `util.inspect` and the old behavior is preserved as
well.
@BridgeAR BridgeAR force-pushed the odd-to-string-behavior branch from 9700e97 to 0bb8a7f Compare May 16, 2019 13:43
@BridgeAR
Copy link
Member Author

BridgeAR commented May 16, 2019

Rebased due to a92ad36.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 16, 2019

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

BridgeAR added a commit to BridgeAR/node that referenced this pull request May 20, 2019
@BridgeAR
doc: update util.format formatters documentation
182b48a 
This includes the information that some inputs are handled
differently than others (e.g., `Symbols` are partially represented
as `NaN`).

PR-URL: nodejs#27621
Reviewed-By: Roman Reiss <[email protected]>
BridgeAR added a commit to BridgeAR/node that referenced this pull request May 20, 2019
@BridgeAR
util: if present, fallback to toString using the %s formatter
5518664 
This makes sure that `util.format` uses `String` to stringify an object
in case the object has an own property named `toString` with type
`function`. That way objects that do not have such function are still
inspected using `util.inspect` and the old behavior is preserved as
well.

PR-URL: nodejs#27621
Refs: jestjs/jest#8443
Reviewed-By: Roman Reiss <[email protected]>
@BridgeAR
Copy link
Member Author

BridgeAR commented May 20, 2019

Landed in 182b48a and 5518664 🎉

@BridgeAR BridgeAR closed this May 20, 2019
targos pushed a commit that referenced this pull request May 20, 2019
@BridgeAR @targos
doc: update util.format formatters documentation
378f44c 
This includes the information that some inputs are handled
differently than others (e.g., `Symbols` are partially represented
as `NaN`).

PR-URL: #27621
Reviewed-By: Roman Reiss <[email protected]>
targos pushed a commit that referenced this pull request May 20, 2019
@BridgeAR @targos
util: if present, fallback to toString using the %s formatter
10b4a81 
This makes sure that `util.format` uses `String` to stringify an object
in case the object has an own property named `toString` with type
`function`. That way objects that do not have such function are still
inspected using `util.inspect` and the old behavior is preserved as
well.

PR-URL: #27621
Refs: jestjs/jest#8443
Reviewed-By: Roman Reiss <[email protected]>
@BridgeAR BridgeAR mentioned this pull request May 21, 2019
Merged
4 tasks
@addaleax addaleax mentioned this pull request Aug 27, 2019
Closed
@cpcallen cpcallen mentioned this pull request Nov 8, 2019
Closed
@BridgeAR BridgeAR deleted the odd-to-string-behavior branch January 20, 2020 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleax addaleax Awaiting requested review from addaleax

@Fishrock123 Fishrock123 Awaiting requested review from Fishrock123

@antsmartian antsmartian Awaiting requested review from antsmartian

1 more reviewer

@silverwind silverwind silverwind 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 pending requests for changes, and a CI started. review wanted PRs that need reviews. 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.

5 participants

@BridgeAR @nodejs-github-bot @silverwind @addaleax @Fishrock123