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.

Sign up for GitHub

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

Jump to bottom

Bind error handler to instance (v2) #2305

Merged
merged 7 commits into from
Jun 2, 2020
Merged

Bind error handler to instance (v2) #2305

merged 7 commits into from
Jun 2, 2020

Conversation

greguz
Copy link
Contributor

@greguz greguz commented May 28, 2020

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
mcollina approved these changes May 28, 2020
View reviewed changes
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

@mcollina mcollina requested a review from delvedor May 28, 2020 16:18
@mcollina
Copy link
Member

@delvedor do you remember why we didn't do this in the first place?

delvedor
delvedor previously requested changes May 28, 2020
View reviewed changes
Copy link
Member

@delvedor delvedor left a comment •
edited

Choose a reason for hiding this comment

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

Can you add another test to verify the encapsulation as well?
You can add a nested error handler and check inside a decorator that should be accessible only inside a plugin.

@mcollina I don't remember :P

@greguz
Copy link
Contributor Author

greguz commented May 28, 2020

@delvedor You're right.

Eomm
Eomm reviewed May 28, 2020
View reviewed changes
test/request-error.test.js Outdated Show resolved Hide resolved
test/request-error.test.js Outdated Show resolved Hide resolved
@Eomm Eomm added semver-minor Issue or PR that should land as semver minor v2.x Issue or pr related to Fastify v2 labels May 28, 2020
@mcollina
Copy link
Member

@greguz I see this targets v2. Is this not a bug in v3?

greguz and others added 2 commits May 29, 2020 08:56
@greguz
Copy link
Contributor Author

greguz commented May 29, 2020 •
edited

@mcollina I saw the v2.x label inside the issue, so I assumed to do the v2 version firstly.

@mcollina
Copy link
Member

Can you send a Fix for v3 as well? Thanks.

@greguz
Copy link
Contributor Author

greguz commented May 29, 2020

NP

@greguz greguz changed the title Bind error handler to instance Bind error handler to instance (v2) May 29, 2020
greguz
greguz commented May 29, 2020
View reviewed changes
Copy link
Contributor Author

@greguz greguz left a comment

Choose a reason for hiding this comment

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

Add encapsulation test in the next commit.

mcollina
mcollina approved these changes May 29, 2020
View reviewed changes
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

Eomm
Eomm approved these changes May 30, 2020
View reviewed changes
Copy link
Member

@Eomm Eomm left a comment

Choose a reason for hiding this comment

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

LGTM the code

Could you add a notice to the doc also?

greguz
greguz commented May 30, 2020
View reviewed changes
Copy link
Contributor Author

@greguz greguz left a comment

Choose a reason for hiding this comment

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

Updated setErrorHandler docs in the next commit.

@mcollina
Copy link
Member

@delvedor are you ok?

jsumners
jsumners reviewed May 30, 2020
View reviewed changes
docs/Server.md Outdated Show resolved Hide resolved
@greguz @jsumners
Update docs/Server.md
117ccb1 
Co-authored-by: James Sumners <james@sumners.email>
@mcollina mcollina dismissed delvedor’s stale review June 2, 2020 09:09

The test was added

@mcollina mcollina merged commit 39e2375 into fastify:2.x Jun 2, 2020
@fralonra fralonra mentioned this pull request Jun 8, 2020
Merged
@greguz greguz mentioned this pull request Jun 9, 2020
Closed
@fralonra fralonra mentioned this pull request Jun 11, 2020
Open
21 tasks
@miwnwski miwnwski mentioned this pull request Oct 1, 2020
Merged
4 tasks
@dependabot dependabot bot mentioned this pull request Mar 11, 2021
Open
@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 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jsumners jsumners jsumners left review comments

@mcollina mcollina mcollina approved these changes

@Eomm Eomm Eomm approved these changes

@delvedor delvedor delvedor left review comments

Assignees
No one assigned
Labels
semver-minor Issue or PR that should land as semver minor v2.x Issue or pr related to Fastify v2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@greguz @mcollina @jsumners @delvedor @Eomm