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

Fix encapsulation of 404 handlers #839

Merged
merged 1 commit into from Mar 8, 2018

Conversation

Projects
None yet
4 participants
@nwoltman
Copy link
Member

commented Mar 7, 2018

Almost all of the new tests included in this change would have failed before this fix.

For example, using a 404 Error to invoke the default 404 handler from a route within a prefixed plugin wasn't working before:

const fastify = require('fastify')()

fastify.register(function (instance, opts, next) {
  instance.get('/path', function (request, reply) {
    reply.send(Object.assign(new Error(), { status: 404 }))
  })
  next()
}, { prefix: '/v1' })

fastify.inject('/v1/path', (err, res) => {
  // TypeError: Cannot read property 'handler' of null
})

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
Fix encapsulation of 404 handlers
Almost all of the new tests included in this change would have failed before this fix.

For example, using a 404 Error to invoke the default 404 handler from a route within a prefixed plugin wasn't working before.
@mcollina
Copy link
Member

left a comment

LGTM, thanks for fixing this.

@allevo
Copy link
Member

left a comment

Why should 404 not found handler be set based on the router instead of based on the plugin?

@nwoltman

This comment has been minimized.

Copy link
Member Author

commented Mar 8, 2018

@allevo It can't be based on the router because a 404 handler is only called if the router can't find a route, which is why there's a fallback router just for the 404 handlers.

@delvedor
Copy link
Member

left a comment

LGTM

@delvedor delvedor added the bugfix label Mar 8, 2018

@delvedor delvedor merged commit 0d0d264 into fastify:master Mar 8, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

instance.register((instance2B, options, next) => {
try {
instance2B.setNotFoundHandler(() => {})

This comment has been minimized.

Copy link
@allevo

allevo Mar 8, 2018

Member

I don't agree here. Two different plugins can have different 404 handler.

A plugin doesn't know where is mounted. This breaks plugin encapsulation.

This comment has been minimized.

Copy link
@nwoltman

nwoltman Mar 8, 2018

Author Member

If the two plugins each had their own 404 handler, what two URLs could you use match each of the handlers?

@allevo

This comment has been minimized.

Copy link
Member

commented Mar 8, 2018

Too late

@delvedor

This comment has been minimized.

Copy link
Member

commented Mar 8, 2018

@allevo write a test an open another pr :)

@nwoltman nwoltman deleted the nwoltman:fix-404-error branch Mar 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.