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.

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

util: format specifier %d use parseInt #23321

Closed
wants to merge 2 commits into from

Conversation

shisama
Copy link
Contributor

@shisama shisama commented Oct 8, 2018

Change spec of %d specifier in util.format to follow whatwg spec
(https://console.spec.whatwg.org/#formatting-specifiers).
This is a breaking change.

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

Fixes: #22885

@nodejs-github-bot nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Oct 8, 2018
@richardlau richardlau added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 8, 2018
@BridgeAR
Copy link
Member

BridgeAR commented Oct 8, 2018

@nodejs/util @nodejs/tsc PTAL

I am somewhat on the edge about this. It has always been like this in Node.js and AFAIK there was never any complaint about it. But following the spec does seem to be the right thing in the end.

@Trott
Copy link
Member

Trott commented Oct 8, 2018

Assuming CITGM is clean and gzemnid doesn't find any big surprises:

I'm in favor of this, but not enough to want to push it through over the objections of other TSC folks should there be any. Call it a +0.5. So I'm going to wait a bit and see what others say...

@thefourtheye
Copy link
Contributor

Can we go through deprecation cycle?

@Trott
Copy link
Member

Trott commented Oct 9, 2018

Can we go through deprecation cycle?

I don't think that's possible here without making things much worse for the end user (e.g., removing %d entirely for a major release, then adding it back in the next major release with the new behavior, resulting in two breaking changes for users).

We're changing behavior, but not deprecating a feature. If it's possible to detect that someone is using %d expecting the current behavior rather than the behavior added in this PR, then I guess we can have a deprecation cycle where that emits a warning. But I'm not sure it's possible to do that reliably.

@thefourtheye
Copy link
Contributor

If it's possible to detect that someone is using %d expecting the current behavior rather than the behavior added in this PR, then I guess we can have a deprecation cycle where that emits a warning. But I'm not sure it's possible to do that reliably.

Yup, this is what I had in mind. Detect if the user is passing a floating point number for %d and start emitting a warning.

@Trott
Copy link
Member

Trott commented Oct 9, 2018

Yup, this is what I had in mind. Detect if the user is passing a floating point number for %d and start emitting a warning.

I would think console.log('%d', 9.55) is a legitimate thing to do and expect to see 9 in the terminal?

@thefourtheye
Copy link
Contributor

I would think console.log('%d', 9.55) is a legitimate thing to do and expect to see 9 in the terminal?

Ideally, yes. But, today, console.log('%d', 9.55); prints 9.55. If there is not much existing code which relies on this behaviour, we can simply call this one out in the release notes and be done with it.

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.

Will need a rebase because of recent BigInt changes landed.

@shisama shisama force-pushed the util-format-integer branch from 2a16287 to c027028 Compare October 10, 2018 22:57
@shisama
Copy link
Contributor Author

shisama commented Oct 12, 2018

@silverwind Thank you for reviewing. I rebased and fixed bigint test.

@addaleax
Copy link
Member

assert.strictEqual(util.format('%d %d', 42, 43), '42 43');
assert.strictEqual(util.format('%d %d', 42), '42 %d');
assert.strictEqual(
util.format('%d', 1180591620717411303424),
'1.1805916207174113e+21'
'1'
Copy link
Member

Choose a reason for hiding this comment

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

I’m a bit surprised by this one, tbh … at least for me locally, `${parseInt('1180591620717411303424')}` === '1.1805916207174113e+21'? Do you know why there’s a discrepancy?

Interestingly, Firefox seems to provide 0 here, and Chrome the exponential notation…

Copy link
Member

@targos targos Oct 17, 2018

Choose a reason for hiding this comment

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

What happens here is not ${parseInt('1180591620717411303424')} but ${parseInt(1180591620717411303424)} which is equivalent to ${parseInt('1.1805916207174113e+21')} and returns 1.

Copy link
Member

Choose a reason for hiding this comment

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

So … just to make sure, these are browser bugs?

Copy link
Member

@targos targos Oct 17, 2018

Choose a reason for hiding this comment

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

Not sure... It could also be a spec bug because no browser has the "correct" behavior.
To summarize, with console.log('%d', 1180591620717411303424):

  • Edge and Chrome: 1.1805916207174113e+21
  • Firefox: 0
  • Safari: 1

Copy link
Member

Choose a reason for hiding this comment

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

To me it seems like the browsers deviate from the spec in this case. However, it does seem to be a a good idea to follow Chrome and Edge in this case. Especially, since the spec does not seem to follow any browser implementation in this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Safari logs 1

Copy link
Contributor

Choose a reason for hiding this comment

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

1 should be correct according to the latest parseInt spec. NumberToString has to convert the number to expotential notation and then the following spec text applies:

If S contains a code unit that is not a radix-R digit, let Z be the substring of S consisting of all code units before the first such code unit

Let mathInt be the mathematical integer value that is represented by Z

. is not a radix-R digit, so parsing stops there.

Copy link
Contributor

Choose a reason for hiding this comment

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

So in summary for console.log('%d', number):

  • Chrome uses Number
  • Edge uses Number
  • Safari uses parseInt (this is spec-compliant)
  • Firefox uses neither Number nor parseInt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Chrome may not use Number for %d.
at least chrome 70(stable) on my macbook, I think %d is not equal Number.
the results are:
console.log('%d', 1.2): 1
Number(1.2): 1.2

@silverwind
Copy link
Contributor

This is going to need another rebase

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.

So, to re-iterate from #22885 (comment), especially given browser behavior difference:

I'd rather keep the minor difference (which the Node.js ecosystem is probably used to) rather than loose functionality

I actually think we may want to consider this a spec bug and file against the spec repo. The spec is fairly new and was supposed to modal somewhat what browsers did and here it certainly does not. The spec is in the minority.

Masashi Hirano added 2 commits October 21, 2018 00:33
Change spec of %d specifier in util.format to follow whatwg spec
(https://console.spec.whatwg.org/#formatting-specifiers).
This is a breaking change.
@shisama shisama force-pushed the util-format-integer branch from 64a267d to cec2e69 Compare October 20, 2018 15:35
@shisama
Copy link
Contributor Author

shisama commented Oct 23, 2018

@Fishrock123 Thank you for your comment. As you think, it may be spec bug.
However, I think it is the right thing that Node.js always follows the specs as far as it can.

@silverwind
Copy link
Contributor

The first aim should be to be as compatible as possible. Specs help with that but they are not worth much if no one's going to follow them, which seems to be the case here. I'd suggest we first investigate the current behaviour of all format specifiers in browsers and node and then present the results to WHATWG so they can evaluate possible spec changes.

@Trott
Copy link
Member

Trott commented Nov 16, 2018

I'd suggest we first investigate the current behaviour of all format specifiers in browsers and node and then present the results to WHATWG so they can evaluate possible spec changes.

@silverwind Is anyone doing that? If not, should we close this since it's not going to reach consensus? (Only other real alternative is to escalate to the TSC.)

@silverwind
Copy link
Contributor

I personally don't have the motivation to do that kind of work for a feature I don't need/use. I think template strings are superior to format specifiers in every way. So I would agree to closing this and related issues.

@shisama
Copy link
Contributor Author

shisama commented Nov 25, 2018

@Trott @silverwind OK, thanks. I close this. Thank you all for your reviews.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major PRs that contain breaking changes and should be released in the next major version. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

util: spec of %d specifier