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

buffer,error: improve bigint, big numbers and more #27228

Closed

Conversation

BridgeAR
Copy link
Member

This improves the error message from ERR_OUT_OF_RANGE by closer
inspecting the value and logging numbers above 2 ** 32 by using
toLocaleString and a similar output for bigint. BigInt is now also
marked if used.

Buffer errors also format the range as 2 ** n instead of showing a
huge number.

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

This comment has been minimized.

@nodejs-github-bot nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core. labels Apr 14, 2019
@joyeecheung
Copy link
Member

Why not use https://github.com/tc39/proposal-numeric-separator instead? Then the number displayed can be copy-pasted and evaluated again when our copy of v8 ships it. (It's already shipped in the upstream)

@BridgeAR
Copy link
Member Author

@joyeecheung

Why not use tc39/proposal-numeric-separator instead?

How does that apply here? If I read the spec correct it's only about allowing underscores in numbers in code. But this changes the way they are displayed in our errors.

@joyeecheung
Copy link
Member

joyeecheung commented Apr 15, 2019

@BridgeAR Sorry, I was not being clear, I meant using the format of the proposal (that is, using underscores) instead of using toLocaleString() (usually commas).

@BridgeAR
Copy link
Member Author

@joyeecheung ah, I don't think that's a good idea. AFAIK the format is used in code because it is not used for anything else in program code (the dot is used as decimal separator). The error is of course meant for developers but I think commas are still nicer to read.

@joyeecheung
Copy link
Member

joyeecheung commented Apr 15, 2019

The error is of course meant for developers but I think commas are still nicer to read.

But the syntax in the spec is also meant for developers? It seems when Node.js is built with full ICU the return value of toLocaleString() depends on the setting of the system and from the examples on MDN some of them look a bit weird in the context of computer programs. It's quite subjective but I personally prefer a format I could paste into (= hard-code in) the code/REPL if I want to fix an error with them.

This improves the error message from `ERR_OUT_OF_RANGE` by closer
inspecting the value and logging numbers above 2 ** 32 by adding
commas to the output for integer and bigint. BigInt is now also
marked if used.

Buffer errors also format the range as 2 ** n instead of showing a
huge number.
@BridgeAR BridgeAR force-pushed the improve-err-out-of-range-message branch from 3e88505 to 8dd4ba0 Compare April 15, 2019 16:51
@BridgeAR
Copy link
Member Author

I updated the PR not to use toLocaleString anymore. PTAL.

@BridgeAR
Copy link
Member Author

@joyeecheung

It's quite subjective but I personally prefer a format I could paste into (= hard-code in) the code/REPL if I want to fix an error with them.

I see. That does make sense. I suggest we could change the format as soon as Node.js actually supports the underscores. Before that, I fear it would confuse people more than help?

Copy link
Member

@richardlau richardlau left a comment

Choose a reason for hiding this comment

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

The Travis commit message lint failure is because the error subsystem should be errors.

@nodejs-github-bot

This comment has been minimized.

@joyeecheung
Copy link
Member

I see. That does make sense. I suggest we could change the format as soon as Node.js actually supports the underscores. Before that, I fear it would confuse people more than help?

SGTM, though I believe that's going to be just a month from now since it's shipped in M75. https://chromiumdash.appspot.com/schedule

@BridgeAR
Copy link
Member Author

@joyeecheung

SGTM, though I believe that's going to be just a month from now since it's shipped in M75.

But it takes time until we actually ship that and until users get to know the new notation. I don't think switching right away is best. And this would only be possible for Node.js v12 while this only changes the error message and is a patch.

@nodejs-github-bot

This comment has been minimized.

@joyeecheung
Copy link
Member

joyeecheung commented Apr 16, 2019

But it takes time until we actually ship that and until users get to know the new notation. I don't think switching right away is best. And this would only be possible for Node.js v12 while this only changes the error message and is a patch.

I think Node.js is in the position to help promoting new features like this, instead of waiting for its adoption, considering this is a runtime where many developers get to try new language features first without transpilation - when there is a new release comes out with a V8 update we tend to mention what language feature is now available and AFAIK many people get to know that a feature is ready through this.

Since it's already shipped in the upstream, it's quite unlikely that this feature is going away, and we are not in the position to control what v8 unflags by default so it's going out soon, it seems a bit odd to me that Node.js itself has to wait for adoption considering it is a channel to promote adoption - user land frameworks (both frontend and backend) are even more agressive in terms of promoting new features that are not even stable in the standardization process.

@BridgeAR
Copy link
Member Author

@joyeecheung if you think that's the best way: I am fine to change it for Node.js v12 and up but this PR is not semver-major and can be backported. I think it's best to only use that format in case that Node.js version actually supports it.

@joyeecheung
Copy link
Member

If this is changing the output from a format that can be copy-pasted in code into something that cannot...I am not sure it's actually an improvement?

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 23, 2019

@BridgeAR BridgeAR requested review from addaleax, richardlau and joyeecheung and removed request for richardlau April 24, 2019 23:22
@nodejs-github-bot
Copy link
Collaborator

lib/internal/errors.js Outdated Show resolved Hide resolved
lib/internal/errors.js Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@BridgeAR
Copy link
Member Author

@bnoordhuis @richardlau would either of you be so kind to confirm your LG due to the changed implementation? This should otherwise be good to go.

@BridgeAR
Copy link
Member Author

This could use one more LG before landing / confirming the LG.

// cc @nodejs/buffer

lib/internal/errors.js Show resolved Hide resolved
BridgeAR added a commit to BridgeAR/node that referenced this pull request May 1, 2019
This improves the error message from `ERR_OUT_OF_RANGE` by closer
inspecting the value and logging numbers above 2 ** 32 by adding
commas to the output for integer and bigint. BigInt is now also
marked if used.

Buffer errors also format the range as 2 ** n instead of showing a
huge number.

PR-URL: nodejs#27228
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@BridgeAR
Copy link
Member Author

BridgeAR commented May 1, 2019

Thanks for the reviews.

Landed in 4416127 🎉

@BridgeAR BridgeAR closed this May 1, 2019
targos pushed a commit that referenced this pull request May 4, 2019
This improves the error message from `ERR_OUT_OF_RANGE` by closer
inspecting the value and logging numbers above 2 ** 32 by adding
commas to the output for integer and bigint. BigInt is now also
marked if used.

Buffer errors also format the range as 2 ** n instead of showing a
huge number.

PR-URL: #27228
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@targos targos mentioned this pull request May 6, 2019
deermichel pushed a commit to electron/node that referenced this pull request Jul 16, 2019
This improves the error message from `ERR_OUT_OF_RANGE` by closer
inspecting the value and logging numbers above 2 ** 32 by adding
commas to the output for integer and bigint. BigInt is now also
marked if used.

Buffer errors also format the range as 2 ** n instead of showing a
huge number.

PR-URL: nodejs/node#27228
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@BridgeAR BridgeAR deleted the improve-err-out-of-range-message branch January 20, 2020 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. buffer Issues and PRs related to the buffer subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core. review wanted PRs that need reviews.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants