-
Notifications
You must be signed in to change notification settings - Fork 660
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
feat: Allow custom express handler after deployd middleware #869
feat: Allow custom express handler after deployd middleware #869
Conversation
Add example of custom fallback handler in test-attach.js
Tests seem to be failing, not sure if it is related to this PR or some other issue. Did you get tests to run locally? |
Looks like this parts don't work for me.
|
Using a custom docker env, I did manage to run the tests :
But still i have some troubles whit one test, but I'm pretty sure it's related to my changes.
|
Is not related, forked the repo, installed deps and run 'npm run test' and the same error on the same lines. |
@christianguevara a fix for those problems was merged. There are still some problems with this PR though. |
I did just fix the last tests, can you give another review ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -31,8 +31,7 @@ Files.prototype.handle = function (ctx, next) { | |||
|
|||
send(ctx.req, url.parse(ctx.url).pathname, {root: path.resolve(this.public)}) | |||
.on('error', function (err) { | |||
ctx.res.statusCode = 404; | |||
respond('Resource Not Found', ctx.req, ctx.res); | |||
return next(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens with inexistent resources here? Do we still return 404 resource not found somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do return a 404. this handler is only run after no resources can be found.
I would like to let express or node http server to handle 404 & eventual redirects AFTER deployd routing consider unable to handle the request.
Description
We look into the first
handleRequest
call to check if their is a next() handler function. If it as one, we consider that the app is part of an custom deployment script or an express one. We do pass the native handler to deployd router to override deployd default 404 handler.Motivation and Context
I would redirecting all non handled request to my own express logic afterward the request had been pass through deployd routing. On some other projects make custom 404 pages this way.
For exemple, attaching an express handler AFTER the deployd middleware :
But we can see many more examples.
How Has This Been Tested?
Don't exactly how test it, I do have updated the
test-attach.js
script but I don't see any test running it...Screenshots (if appropriate):
Types of changes
Possible breaking changes if we consider changing the final handling behavior of deployd routing.
Checklist: