Feature/#909 error codes by StarpTech · Pull Request #957 · fastify/fastify

StarpTech · 2018-06-02T19:22:03Z

This is a first draft of introducing error codes to fastify. It only handles the codes for the contentTypeParser.

The old PR discussion is here

TODO

Move error message and code to error

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

mcollina · 2018-06-03T06:57:24Z

This is good for me. It needs all errors to be copied and docs.

allevo

Nice work! I like it.

Only a suggestion

allevo · 2018-06-03T10:38:54Z

+      return `FastifyError [${code}]`
+    }
+    get code () {
+      return code


Can this property set to the prototype?

What do you mean? It is part of the prototype. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/get#get_Vs._defineProperty

FastifyError.prototype.code = code

That's the same. http://jsbin.com/qaxawoheji/edit?js,console

allevo

Can you list all errors in the doc?

nwoltman · 2018-06-03T16:13:47Z

+
+function createMessage (msg, args = []) {
+  args.unshift(msg)
+  if (args.length === 0) return msg


This line should be before args.unshift(msg), otherwise args.length will always be at least 1.

nwoltman · 2018-06-03T16:18:45Z

+  code = code.toUpperCase()
+  codes[code] = class FastifyError extends Base {
+    constructor (...args) {
+      super(`Code: ${code}; ${createMessage(msg, args)}`)


As an alternative to my comment above, you could use util.format() here directly. It already includes a check similar to the one you have in createMessage().

Good addition. I will remove it.

delvedor · 2018-06-04T14:09:38Z

+
+function createError (code, msg, statusCode = 500, Base = Error) {
+  if (!code) throw new Error('Fastify error code must not be empty')
+  if (!msg) throw new Error('Fastify error message must not be empty')


If you pass an empty string this will throw, is it ok?
Can you use more strict checks?

I think that's enough. I just want to avoid that an empty value (0, '') or no value is passed.

delvedor

Great work!
Just a question, why are you using the class syntax?

Personally, I prefer to stick classic one, for two main reasons:

in Fastify we aren’t using (es6) classes anywhere, while we are using JavaScript prototype-based inheritance (see request, reply…)
class syntax is just syntactic sugar around the classic JavaScript prototype-based inheritance

Would the following code produce the same result?

const { inherits } = require('util')

function FastifyError (code, message) {
  Error.call(this)
  Error.captureStackTrace(this, FastifyError)
  this.name = `FastifyError [${code}]`
  this.code = code
  this.message = message
}

inherits(FastifyError, Error)

Remove eslint ignore comment

* Remove/Merge redundant decorate functions * Should return this instead of prototype * Simplify existence check

Add more details about the baseline ajv configuration

…ndlers (fastify#1134)

mcollina · 2018-09-11T09:23:35Z

@StarpTech ping

StarpTech · 2018-09-12T19:08:14Z

Hi @mcollina my time is very limited right now. I will try to continue my work on the weekend. Thanks for pointing me out.

…tify#1162) * added default Query, Params, Headers, and Body types * fixed contentTypeParser types * added additional tests for default Buffer body parser * fixed tests * allow array of strings in addContentTypeParser

* added default Query, Params, Headers, and Body types * added tests * added tests for generics * added generics info to docs

* Added error handling doc * Updated based on comments * Update 2, based on comments * Updated based on comments

mcollina · 2018-09-18T19:39:54Z

There is something off with this branch. You should probably rebase over next.

This reverts commit 4f0dca1, reversing changes made to d60680e.

StarpTech · 2018-09-18T19:50:42Z

I will create a new one.

github-actions · 2022-02-14T00:14:59Z

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.

StarpTech added 18 commits April 27, 2018 13:47

add error codes

2f0b1c7

[ci skip] add comment

465e13f

Merge branch 'master' into feature/#909_error_codes

a66bc91

first proposal of fastify errors

016228a

simplify

ba02fad

[ci skip] add example doc

ee34bad

[ci skip] update doc

0505b4e

include code in err msg

e82fa58

split in type and range errors, use clean names

62c1721

avoid promise creation

791de5f

verify code on error

6be98ec

assign statusCode and error message at creation time

deee08f

Merge branch 'master' into feature/#909_error_codes

38bb543

statusCode is optional

ff99b21

validate required err params

5110c95

avoid var

0e5fcc5

Merge branch 'master' into feature/#909_error_codes

89e3927

Merge branch 'master' into feature/#909_error_codes

3bec0c3

StarpTech requested review from allevo, delvedor, jsumners, mcollina and nwoltman June 2, 2018 19:22

allevo reviewed Jun 3, 2018

View reviewed changes

nwoltman reviewed Jun 3, 2018

View reviewed changes

use util.format directly

8cca10d

delvedor reviewed Jun 4, 2018

View reviewed changes

cemremengu and others added 14 commits August 30, 2018 17:42

Update ContentTypeParser.js (fastify#1122)

7c8401d

Remove eslint ignore comment

Updated standard to v12. (fastify#1121)

baeebef

Remove/Merge redundant decorate functions (fastify#1120)

d6982ea

* Remove/Merge redundant decorate functions * Should return this instead of prototype * Simplify existence check

Update Validation-and-Serialization.md (fastify#1124)

60b85e7

Add more details about the baseline ajv configuration

Add test for error fixed in fastify/avvio#74 (fastify#1132)

6d302a5

Rename ContentTypeParser (fastify#1123)

cce1a85

Handle promises in the error handler with the same logic of normal ha…

0a874b9

…ndlers (fastify#1134)

Updated ecosystem doc (fastify#1137)

5b18645

Correct typos in Github Issue Template (fastify#1140)

0a27c92

Add trivikr@ to the collaborators list (fastify#1139)

c40ea62

Bumped v1.11.2

4e047a8

Add fastify-webpack-hmr community plugin to ECOSYSTEM.md (fastify#1152)

604ad47

Add badge of vulnerabilities from snyk.io (fastify#1149)

239e4a2

Add fastify-loader to Ecosystem.md (fastify#1154)

9e11a69

TheNoim and others added 2 commits September 11, 2018 11:25

Add fastify-vue-plugin to Ecosystem.md (fastify#1151)

b469805

Updated based on fastify#534 (fastify#1157)

d0d2aa9

jineshshah36 and others added 4 commits September 16, 2018 20:19

added generic Query, Params, Headers, and Body types (fastify#1160)

15fbe24

* added default Query, Params, Headers, and Body types * added tests * added tests for generics * added generics info to docs

Error Handling Documentation (fastify#1130)

4b5f818

* Added error handling doc * Updated based on comments * Update 2, based on comments * Updated based on comments

Merge branch 'master' into feature/#909_error_codes

4f0dca1

Revert "Merge branch 'master' into feature/#909_error_codes"

d071ed9

This reverts commit 4f0dca1, reversing changes made to d60680e.

StarpTech closed this Sep 18, 2018

StarpTech mentioned this pull request Sep 18, 2018

Feature/better error management #1168

Merged

6 tasks

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

Eomm added the semver-minor Issue or PR that should land as semver minor label Apr 1, 2023

Uh oh!

Conversation

StarpTech commented Jun 2, 2018

TODO

Checklist

Uh oh!

mcollina commented Jun 3, 2018

Uh oh!

allevo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allevo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

delvedor Jun 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

delvedor left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Sep 11, 2018

Uh oh!

StarpTech commented Sep 12, 2018

Uh oh!

mcollina commented Sep 18, 2018

Uh oh!

StarpTech commented Sep 18, 2018

Uh oh!

github-actions Bot commented Feb 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

20 participants

delvedor Jun 4, 2018 •

edited

Loading