Remove code from error name by jsumners · Pull Request #1786 · fastify/fastify

jsumners · 2019-08-04T12:25:02Z

Run the following under Node 12:

'use strict'

try {
  const buf = Buffer.alloc(Infinity)
} catch (error) {
  console.log(error)
  console.log('-----------')
  console.log(error.name)
  console.log(error.message)
  console.log(error.code)
}

You will get the following output:

RangeError [ERR_INVALID_OPT_VALUE]: The value "Infinity" is invalid for option "size"
    at Function.alloc (buffer.js:259:3)
    at Object.<anonymous> (/private/var/folders/n6/3nctw4k120ddw_jyvxkjlz840000gn/T/CodeRunner/Untitled.js:4:22)
    at Module._compile (internal/modules/cjs/loader.js:777:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:788:10)
    at Module.load (internal/modules/cjs/loader.js:643:32)
    at Function.Module._load (internal/modules/cjs/loader.js:556:12)
    at Function.Module.runMain (internal/modules/cjs/loader.js:840:10)
    at internal/main/run_main_module.js:17:11
-----------
RangeError
The value "Infinity" is invalid for option "size"
ERR_INVALID_OPT_VALUE

Notice that error.name does not include error.code. Node's default error serialization automatically inserts the code into the message. There isn't any need for

fastify/lib/errors.js

Line 81 in 53bac6d

this.name = `FastifyError [${code}]`

^ That just makes it more difficult for downstream users to work with the error objects. This PR removes the code from the names of FastifyError instances.

Checklist

run npm run test and npm run benchmark
tests and/or benchmarks are included
documentation is changed or added
commit message and code follows Code of conduct

StarpTech · 2019-08-04T13:03:03Z

Node's default error serialization automatically inserts the code into the message. There isn't any need for ...

Could you reference to this behaviour in code or doc? I couldn't find anything.

I disagree with that change because the code is an important identifier for the exact fastify errors https://github.com/fastify/fastify/blob/master/docs/Errors.md. The code in the error description is much harder to find e.g in long stack traces, cutted strings etc...

That just makes it more difficult for downstream users to work with the error objects

I don't understand it. Could you elaborate?

StarpTech · 2019-08-04T13:06:48Z

Therefore you example is only valid for node core issues. In userland errors you will get this! No error code!

    const e = new Error('#######')
    e.code = "dede"
    throw e

Error: #######
    at Object.<anonymous> (/mnt/ssd/repositories/rbtx/rbtx-web/test.js:2:17)
    at Module._compile (internal/modules/cjs/loader.js:816:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:827:10)
    at Module.load (internal/modules/cjs/loader.js:685:32)
    at Function.Module._load (internal/modules/cjs/loader.js:620:12)
    at Function.Module.runMain (internal/modules/cjs/loader.js:877:12)
    at internal/main/run_main_module.js:21:11

Only if you print the err object you will see the code at the end of the stack trace

try {
    const e = new Error('#######')
    e.code = "dede"
    throw e
  } catch (error) {
    console.log(error)
    console.log('-----------')
    console.log(error.name)
    console.log(error.message)
    console.log(error.code)
  }


{ Error: #######
    at Object.<anonymous> (/mnt/ssd/repositories/rbtx/rbtx-web/test.js:2:17)
    at Module._compile (internal/modules/cjs/loader.js:701:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:712:10)
    at Module.load (internal/modules/cjs/loader.js:600:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:539:12)
    at Function.Module._load (internal/modules/cjs/loader.js:531:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:754:12)
    at startup (internal/bootstrap/node.js:283:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:622:3) code: 'dede' }

Do I miss smt?

jsumners · 2019-08-04T15:33:24Z

People parse errors and expect the .name to be the name of the error not the name and the error code. As an example, if you are preparing a payload to send to BugSnag then you’ll need to send a errorClass property. This property directly correlates to the .name of a JavaScript error.

The simple fact is: FastifyError is not standard because of the way it builds the .name. This PR fixes it.

StarpTech · 2019-08-04T16:10:32Z

People parse errors and expect the .name to be the name of the error not the name and the error code.

Which people? Issues?

I just expect a useful name and the error code will help to identify it.

If you need a constant name you could rely on the constructor name. I have never heard that the error name must be a fixed across all errors. ~~The node core promotes this pattern as well.~~

jsumners · 2019-08-04T16:30:21Z

I could list packages that parse errors all day long.

This PR does not remove the error code. The error code is still there to provide the utility it is meant to provide: insight into the specific situation that caused the error.

StarpTech · 2019-08-04T16:37:44Z

I have no strong opinion about that change but I like it as it is because it's easy too identify. How is the error logged in fastify? (can't test right now) Is the fastify error code printed? When yes +1

StarpTech · 2019-08-04T17:19:02Z

+  t.plan(1)
+  const NewError = createError('CODE', 'foo')
+  const err = new NewError()
+  t.equal(err.toString(), 'FastifyError [CODE]: foo')


Does the default error serializer rely on toString?

StarpTech

LGTM

mcollina

LGTM

mcollina · 2019-08-04T18:30:40Z

This is tagged as semver-major, but is that so? Errors are identified by code, so that we can change the name without breakage.

allevo

LGTM

mcollina

LGTM

jsumners · 2019-08-04T18:57:13Z

I wrote it against next because it changes the default serialization of errors when console.log(error). If we don’t think that is a breaking change then I can re-do it on master.

What do you say @mcollina?

Add FastifyError.toString to display code in `console.log(error)`

jsumners · 2019-08-04T21:35:29Z

See #1787 for this PR against master instead of next.

delvedor

LGTM

Add FastifyError.toString to display code in `console.log(error)`

github-actions · 2022-02-10T09:14:11Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

jsumners added semver-major Issue or PR that should land as semver major internals Change that won't impact the surface API. labels Aug 4, 2019

StarpTech reviewed Aug 4, 2019

View reviewed changes

StarpTech approved these changes Aug 4, 2019

View reviewed changes

mcollina approved these changes Aug 4, 2019

View reviewed changes

allevo approved these changes Aug 4, 2019

View reviewed changes

mcollina approved these changes Aug 4, 2019

View reviewed changes

Remove code from error name

54c2b78

Add FastifyError.toString to display code in `console.log(error)`

jsumners force-pushed the error-code-separation branch from 5fd7d66 to 54c2b78 Compare August 4, 2019 21:12

jsumners mentioned this pull request Aug 4, 2019

Remove code from error name (v2.x) #1787

Closed

4 tasks

delvedor approved these changes Aug 5, 2019

View reviewed changes

jsumners merged commit 2393c45 into next Aug 9, 2019

jsumners deleted the error-code-separation branch August 9, 2019 22:51

Eomm pushed a commit to Eomm/fastify that referenced this pull request Nov 24, 2019

Remove code from error name (fastify#1786)

24cf33d

Add FastifyError.toString to display code in `console.log(error)`

mcollina mentioned this pull request Nov 24, 2019

Rebase next #1953

Closed

4 tasks

github-actions Bot locked as resolved and limited conversation to collaborators Feb 10, 2022

Uh oh!

Conversation

jsumners commented Aug 4, 2019

Checklist

Uh oh!

StarpTech commented Aug 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

StarpTech commented Aug 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsumners commented Aug 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

StarpTech commented Aug 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsumners commented Aug 4, 2019

Uh oh!

StarpTech commented Aug 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

StarpTech Aug 4, 2019

Choose a reason for hiding this comment

Uh oh!

jsumners Aug 4, 2019

Choose a reason for hiding this comment

Uh oh!

StarpTech left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Aug 4, 2019

Uh oh!

allevo left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

jsumners commented Aug 4, 2019

Uh oh!

jsumners commented Aug 4, 2019

Uh oh!

delvedor left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Feb 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

StarpTech commented Aug 4, 2019 •

edited

Loading

StarpTech commented Aug 4, 2019 •

edited

Loading

jsumners commented Aug 4, 2019 •

edited

Loading

StarpTech commented Aug 4, 2019 •

edited

Loading

StarpTech commented Aug 4, 2019 •

edited

Loading