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

Feature/#235 pass payload to extend server error #236

Conversation

StarpTech
Copy link
Member

@StarpTech StarpTech commented Sep 17, 2017

I removed the validation for the response type of the extendServerError because this would require that the user has to check that err is set although this method is only called when an error is responded. In my opinion, its the task of the dev whether he uses the interface correctly. We can discuss it here.

Copy link
Member

@delvedor delvedor left a comment

Choose a reason for hiding this comment

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

Awesome, just few things :)

@@ -51,10 +51,6 @@ function extendServerError (fn) {
throw new TypeError('The server error object must be a function')
}

if (typeof fn() !== 'object' || fn() === null || Array.isArray(fn())) {
Copy link
Member

Choose a reason for hiding this comment

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

I didn't understand why you removed this check.
If you pass something that is not an object, strange things will happen.

For example:

const xtend = require('xtend')
xtend({ hello: 'world' }, 'hello') // { '0': 'h', '1': 'e', '2': 'l', '3': 'l', '4': 'o', hello: 'world' }

Copy link
Member Author

Choose a reason for hiding this comment

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

But is this a task of the framework? The dev should return an object. When I add this check the function is executed although no error was responded. The dev is forced to make a null check in extendServerError

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like the xtend function is too liberal. We use it only for this case. Why we don't switch to Object.assign ?

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I've understood your point, we should demand that check to the user, maybe we can be more clear about this in the docs.

Regarding Object.assign vs xtend we should keep using xtend, since it is way faster.

@@ -201,7 +201,11 @@ test('extend server error - encapsulation', t => {
})

fastify.register((instance, opts, next) => {
instance.extendServerError(() => {
instance.extendServerError((payloadError) => {
t.strictEqual(payloadError instanceof Error, true)
Copy link
Member

Choose a reason for hiding this comment

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

This could be just t.ok.

@@ -58,9 +58,9 @@ Note: using an arrow function will break the binding of `this` to the Fastify `r
<a name="extend-server-error"></a>
**extendServerError**
If you need to extend the standard [server error](https://github.com/fastify/fastify/blob/master/docs/Reply.md#errors), this api is what you need.
You *must* pass a function that returns an object, Fastify will extend the server error with the returned object of your function.
You *must* pass a function that returns an object, Fastify will extend the server error with the returned object of your function. We pass the original error as first argument.
Copy link
Member

Choose a reason for hiding this comment

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

... The function will receive the original error object.

Copy link
Member

@delvedor delvedor left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@delvedor delvedor merged commit 66e1485 into fastify:master Sep 18, 2017
@StarpTech StarpTech deleted the feature/#235_pass_payload_to_extendServerError branch October 14, 2017 16:32
haggholm pushed a commit to haggholm/fastify that referenced this pull request Jun 12, 2021
…/fastify-3.16.2

Bump fastify from 3.16.1 to 3.16.2
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 24, 2022
@Eomm Eomm added the semver-minor Issue or PR that should land as semver minor label Apr 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
semver-minor Issue or PR that should land as semver minor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants