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

Handle BigInt with the %d and %i format specifiers #224

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Handle BigInt with the %d and %i format specifiers #224

wants to merge 1 commit into from

Conversation

domfarolino
Copy link
Member

@domfarolino domfarolino commented Jul 12, 2023 •
edited by pr-preview bot
Loading

This PR aims to close #148 by including special-handling for BigInt in the %d and %i format specifier logic, that defers to the ECMAScript BigInt::toString() abstract operation, which provides a stringified representation of the BigInt that's optimal for that type.

  • Firefox already implements this behavior exactly
  • Safari almost implements this; they add fancy commas that the BigInt::toString() abstract operation doesn't add
  • Node.js also almost implements this PR, but adds the n suffix to the logged output, which this PR does not call for

All of those implementations specifically do not implement what is standardized right now, which is calling %parseInt()% on BigInts formatted with the %d and %i specifier, so the shortest path to interoperability is likely to change the spec here (and nudge other implementations to slightly modify their behavior to match this PR, which they're so close to doing). The only browser that implements the current spec behavior is Chromium, and I personally like what the other implementations are doing better. The ecosystem has seemed to naturally drift in the the direction of the non-Chromium implementation too.

(The below template will be filled out provided this PR is acceptable).

  • At least two implementers are interested (and none opposed):
    • Firefox (already implements this behavior)
    • Safari is quite close. We can see if they'd be willing to change to strictly adhere to this spec
    • Node.js is quite close. We can see if they'd be willing to change to strictly adhere to this spec
  • Tests are written and can be reviewed and commented upon at:
    • Need manual tests
  • Implementation bugs are filed:
    • Chromium: …
    • Gecko: …
    • WebKit: …
    • Deno: …
    • Node.js: …

(See WHATWG Working Mode: Changes for more details.)

Preview | Diff

@domfarolino domfarolino requested a review from terinjokes July 12, 2023 00:24
@domfarolino
Copy link
Member Author

Hey @terinjokes if you could take a look at this when you get time that'd be great. I opted to spec what was described in #148 (comment), which conveniently seems to be almost exactly what most implementations are doing. This is not the direction that you mentioned elsewhere in #148 about adding a new format specifier, but I personally prefer this path forward and would like your feedback on this direction.

@dcrousso
Copy link
Contributor

i think WebKit shows a comma because it uses Number.prototype.toLocaleString for %i/%d/%f

seems reasonable to just remove that (not to mention console.log("1000") should probably have the same output as console.log("%i", 1000))

@domfarolino
Copy link
Member Author

Oh yeah I didn't realize Safari was doing that unconditionally, even for non-BigInts. I agree it seems reasonable to remove that. I've filed https://bugs.webkit.org/show_bug.cgi?id=259146 independently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@terinjokes terinjokes Awaiting requested review from terinjokes

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Format specifier for BigInt
2 participants
@domfarolino @dcrousso