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: if present, fallback to toString using the %s formatter #27621

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

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

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

@nodejs/util PTAL

@nodejs-github-bot
Copy link
Collaborator

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

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.

doc/api/util.md Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

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

@BridgeAR
Copy link
Member Author

@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

@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

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

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

Rebased due to a92ad36.

@nodejs-github-bot
Copy link
Collaborator

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 <me@silverwind.io>
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 <me@silverwind.io>
@BridgeAR
Copy link
Member Author

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 <me@silverwind.io>
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 <me@silverwind.io>
@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

@silverwind silverwind silverwind approved these changes

@addaleax addaleax Awaiting requested review from addaleax

@Fishrock123 Fishrock123 Awaiting requested review from Fishrock123

@antsmartian antsmartian Awaiting requested review from antsmartian

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