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

errors: improve the description of ERR_INVALID_ARG_VALUE #18358

Closed
wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Jan 24, 2018

  • Allow user to customize why the argument is invalid
  • Display the argument with util.inspect so null bytes can be
    displayed properly.

Spinning off from #18308 , but I think this can be submitted alone since that one needs a bit more reviews to land and that's semver-major. The current formatter does not allow users to explain why the argument is invalid and it displays the argument with ${String(value)} which cannot display null bytes properly. This patch makes the error message more debuggable.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

errors

- Allow user to customize why the argument is invalid
- Display the argument with util.inspect so null bytes can be
  displayed properly.
@nodejs-github-bot nodejs-github-bot added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Jan 24, 2018
@joyeecheung
Copy link
Member Author

@joyeecheung
Copy link
Member Author

CI failures look unrelated.

@joyeecheung joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 25, 2018
@joyeecheung
Copy link
Member Author

Landed in 3ec7921, thanks!

@joyeecheung joyeecheung removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 29, 2018
joyeecheung added a commit that referenced this pull request Jan 29, 2018
- Allow user to customize why the argument is invalid
- Display the argument with util.inspect so null bytes can be
  displayed properly.

PR-URL: #18358
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
@MylesBorins
Copy link
Contributor

This does not land cleanly on v9.x, would it be a good idea the backport?

joyeecheung added a commit to joyeecheung/node that referenced this pull request Feb 22, 2018
- Allow user to customize why the argument is invalid
- Display the argument with util.inspect so null bytes can be
  displayed properly.

PR-URL: nodejs#18358
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
MylesBorins pushed a commit that referenced this pull request Feb 26, 2018
- Allow user to customize why the argument is invalid
- Display the argument with util.inspect so null bytes can be
  displayed properly.

Backport-PR-URL: #18916
PR-URL: #18358
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
MylesBorins pushed a commit that referenced this pull request Feb 26, 2018
- Allow user to customize why the argument is invalid
- Display the argument with util.inspect so null bytes can be
  displayed properly.

Backport-PR-URL: #18916
PR-URL: #18358
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
@addaleax addaleax mentioned this pull request Feb 27, 2018
joyeecheung added a commit to joyeecheung/node that referenced this pull request May 2, 2018
PR-URL: nodejs#18358
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
- Allow user to customize why the argument is invalid
- Display the argument with util.inspect so null bytes can be
  displayed properly.

PR-URL: nodejs#18358
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
MylesBorins pushed a commit that referenced this pull request May 22, 2018
Backport-PR-URL: #19191
PR-URL: #18358
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
MylesBorins pushed a commit that referenced this pull request Jun 14, 2018
Backport-PR-URL: #19191
PR-URL: #18358
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
@MylesBorins MylesBorins mentioned this pull request Jul 9, 2018
@codebytere
Copy link
Member

@joyeecheung this doesn't apply cleanly to v8.x either, should it be backported?

rvagg pushed a commit that referenced this pull request Aug 16, 2018
Backport-PR-URL: #19191
PR-URL: #18358
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
@jasnell
Copy link
Member

jasnell commented Aug 17, 2018

I don't believe this one should be backported to 8.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants