Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,17 +36,24 @@ function fastbootExpressMiddleware(distPath, options) {
result.html()
.then(html => {
let headers = result.headers;
let statusMessage = result.error ? 'NOT OK ' : 'OK ';

for (var pair of headers.entries()) {
res.set(pair[0], pair[1]);
}

log(result.statusCode, 'OK ' + path);
if (result.error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Readme update: if you are in resilient mode you delegate the error handling to us.

log("RESILIENT MODE CAUGHT:", result.error.stack);
next(result.error);
Copy link
Member

@danmcclain danmcclain Aug 31, 2016

Choose a reason for hiding this comment

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

We should return after calling next, so that we can no longer alter the response

}

log(result.statusCode, statusMessage + path);
res.status(result.statusCode);
res.send(html);
})
.catch(error => {
console.log(error.stack);
log(500, error.stack);
next(error);
res.sendStatus(500);
Copy link
Member

Choose a reason for hiding this comment

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

Should we res.status here instead, so that middleware can alter the response, instead of being forced to deal with sendStatus

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup. will do.

Copy link
Member

Choose a reason for hiding this comment

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

next should be called last

});
}
Expand All @@ -55,6 +62,7 @@ function fastbootExpressMiddleware(distPath, options) {
if (error.name === "UnrecognizedURLError") {
next();
} else {
next(error);
Copy link
Member

Choose a reason for hiding this comment

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

This should go after the work we do to res, so that middleware can potentially recover

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Look at deleting stuff after next

Copy link
Member

Choose a reason for hiding this comment

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

We think we want to remove 66-71?

log(500, "Unknown Error: " + error.stack);
if (error.stack) {
res.status(500).send(error.stack);
Expand Down
17 changes: 16 additions & 1 deletion test/helpers/test-http-server.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,13 @@ class TestHTTPServer {

app.get('/*', this.middleware);

if (options.errorHandling) {
app.use((err, req, res, next) => {
res.set('x-test-error', 'error handler called');
next();
});
}

return new Promise((resolve, reject) => {
let port = options.port || 3000;
let host = options.host || 'localhost';
Expand All @@ -42,9 +49,17 @@ class TestHTTPServer {
});
}

request(urlPath) {
request(urlPath, options) {
let info = this.info;
let url = 'http://[' + info.host + ']:' + info.port;

if (options && options.resolveWithFullResponse) {
return request({
resolveWithFullResponse: options.resolveWithFullResponse,
uri: url + urlPath
});
}

return request(url + urlPath);
}

Expand Down
50 changes: 49 additions & 1 deletion test/middleware-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ describe("FastBoot", function() {
});
});

it("renders an empty page if the resilient flag is set", function() {
it("renders no FastBoot markup if the resilient flag is set", function() {
let middleware = fastbootMiddleware({
distPath: fixture('rejected-promise'),
resilient: true
Expand All @@ -79,6 +79,54 @@ describe("FastBoot", function() {
});
});

it("propagates to error handling middleware if the resilient flag is set", function() {
let middleware = fastbootMiddleware({
distPath: fixture('rejected-promise'),
resilient: true
});
server = new TestHTTPServer(middleware, { errorHandling: true });

return server.start()
.then(() => server.request('/', { resolveWithFullResponse: true }))
.then(({ body, statusCode, headers }) => {
expect(statusCode).to.equal(200);
expect(headers['x-test-error']).to.match(/error handler called/);
expect(body).to.match(/hello world/);
});
});

it("propagates to error handling middleware if the resilient flag is not set", function() {
let middleware = fastbootMiddleware({
distPath: fixture('rejected-promise'),
resilient: false,
});
server = new TestHTTPServer(middleware, { errorHandling: true });

return server.start()
.then(() => server.request('/', { resolveWithFullResponse: true }))
.catch(({ statusCode, response: { headers } }) => {
expect(statusCode).to.equal(500);
expect(headers['x-test-error']).to.match(/error handler called/);
});
});

it("is does not propagate errors when the reslient flag is set and there is no error handling middleware", function() {
let middleware = fastbootMiddleware({
distPath: fixture('rejected-promise'),
resilient: true,
});
server = new TestHTTPServer(middleware, { errorHandling: false });

return server.start()
.then(() => server.request('/', { resolveWithFullResponse: true }))
.then(({ body, statusCode, headers }) => {
expect(statusCode).to.equal(200);
expect(headers['x-test-error']).to.not.match(/error handler called/);
expect(body).to.not.match(/error/);
expect(body).to.match(/hello world/);
});
});

it("can be provided with a custom FastBoot instance", function() {
let fastboot = new FastBoot({
distPath: fixture('basic-app')
Expand Down