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: remove ERR_INVALID_ARRAY_LENGTH #20484

Closed
wants to merge 1 commit into from

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented May 2, 2018

This error code is obsolete, since the error message from
ERR_OUT_OF_RANGE is more precise. It was only used a single time,
so I went ahead and replced this.

#20483 should land first.

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

@BridgeAR BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 2, 2018
@nodejs-github-bot nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. process Issues and PRs related to the process subsystem. labels May 2, 2018
@BridgeAR BridgeAR requested a review from a team May 2, 2018 21:54
Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

FWIW, I always feel that the error message mentioning only the argument name seems a bit confusing. Not that ERR_INVALID_ARRAY_LENGTH is any better in this regard though.

@BridgeAR
Copy link
Member Author

BridgeAR commented May 2, 2018

@joyeecheung do you have a suggestion how to improve that?

@BridgeAR
Copy link
Member Author

BridgeAR commented May 2, 2018

@joyeecheung
Copy link
Member

joyeecheung commented May 2, 2018

@BridgeAR It would be really helpful if there is something that can generate URLs (on error codes and related API signatures) based on the error. That something does not have to be in core though.

@BridgeAR
Copy link
Member Author

BridgeAR commented May 2, 2018

You mean with a more detailed explanation of the error code? E.g., a reference to our errors.md?

@BridgeAR BridgeAR requested a review from a team May 2, 2018 22:34
@joyeecheung
Copy link
Member

@BridgeAR yeah, but probably it would be better pointing to the HTML for the same release line since signatures can change. It's not really feasible to put too much information in the error message so we might as well show users URLs (like, modules that show users stackoverflow URLs for mistakes and errors seem to be quite popular in the python world). Although that thing probably can just be an npm module.

@joyeecheung
Copy link
Member

joyeecheung commented May 3, 2018

On a second thought maybe metadata like error.api or error.anchor would be useful, although we still change our signature ids (the ones we use for markdown references) from time to time. Maybe it would be useful if we start to assign permanent ids to our APIs like what ecma262 does. (Just throwing ideas, not really related to this PR, sorry if it's totally off-topic)

@BridgeAR
Copy link
Member Author

BridgeAR commented May 3, 2018

@joyeecheung adding a extra property that contains the url to our current release line documentation should be possible. Honestly speaking, I do not think it is the right time to do that though. Often, I find the current documentation in errors.md subpar, especially when trying to give further context for the specific error. It is just to broad and not detailed enough. Otherwise I think it is a good idea. Maybe we can do that in a year or so after porting everything to our error codes and finalizing them.

@joyeecheung
Copy link
Member

joyeecheung commented May 3, 2018

@BridgeAR Hm, to be honest I don't think hard-coding URLs as metadata is a good idea, since URLs can change. Adding permanent API ids as metadata and then generating URLs with it could be, and I think it would help user-land REPLs provide something better for the users, referencing our docs or not. Probably would also help users googling answers on stackoverflow/github issues since there is something you know you can search for meaningful results. There would also be less amount of guesswork when debugging an error without proper stack traces (and the usage of the non-standard Error.captureStackTrace would be less relevant).

@jasnell
Copy link
Member

jasnell commented May 3, 2018

@joyeecheung

@BridgeAR It would be really helpful if there is something that can generate URLs (on error codes and related API signatures) based on the error. That something does not have to be in core though.

And based on the Node.js version. It should point to the documentation for that error message for the particular Node.js version that was used given that obsolete codes are removed.

@addaleax
Copy link
Member

addaleax commented May 5, 2018

@BridgeAR Looks like this might need a rebase?

This error code is obsolete, since the error message from
ERR_OUT_OF_RANGE is more precise. It was only used a single time,
so I went ahead and replced this.
@BridgeAR
Copy link
Member Author

BridgeAR commented May 7, 2018

Rebased.

CI before landing https://ci.nodejs.org/job/node-test-pull-request/14694/

BridgeAR added a commit to BridgeAR/node that referenced this pull request May 7, 2018
This error code is obsolete, since the error message from
ERR_OUT_OF_RANGE is more precise. It was only used a single time,
so I went ahead and replced this.

PR-URL: nodejs#20484
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@BridgeAR
Copy link
Member Author

BridgeAR commented May 7, 2018

Landed in 186857f

@BridgeAR BridgeAR closed this May 7, 2018
@shobhitsharma
Copy link

Crashing quite a few packages like webpack

@mcollina
Copy link
Member

@shobhitsharma on Node 11? Can you please make an example/link to an issue?

@BridgeAR BridgeAR deleted the remove-error-code branch January 20, 2020 11:31
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. process Issues and PRs related to the process subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants