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

Error handling #95

Merged
merged 3 commits into from
Jan 15, 2017
Merged

Error handling #95

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c8d3262
spawn and kill the server in tests
jacekkopecky Jan 13, 2017
854f22e
error handling revamp
jacekkopecky Jan 14, 2017
d0ed31b
don't expect no exit code in the test stopping the server
jacekkopecky Jan 14, 2017
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
62 changes: 33 additions & 29 deletions middleware.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,18 +61,6 @@ module.exports = function(options) {
options = { src: options };
}

var sassMiddlewareError = null;
var cachedErrorCb = options.error;

// This function will be called if something goes wrong
var error = function(err) {
if (cachedErrorCb) {
cachedErrorCb(err);
}

sassMiddlewareError = err;
};

// Source directory (required)
var src = options.src || function() {
throw new Error('sass.middleware() requires "src" directory.');
Expand Down Expand Up @@ -109,6 +97,21 @@ module.exports = function(options) {

// Middleware
return function sass(req, res, next) {
var sassMiddlewareError = null;

// This function will be called if something goes wrong
var error = function(err, errorMessage) {
if (debug) {
log('error', 'error', errorMessage || err);
}

if (options.error) {
options.error(err);
}

sassMiddlewareError = err;
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved error() here so sass() is reentrant.

added logging of all errors if debugging is on

if (req.method != 'GET' && req.method != 'HEAD') {
return next();
}
Expand Down Expand Up @@ -141,24 +144,20 @@ module.exports = function(options) {

// When render is done, respond to the request accordingly
var done = function(err, result) {
var data;

if (err) {
var file = sassPath;
if (err.file && err.file != 'stdin') {
file = err.file;
}

var fileLineColumn = file + ':' + err.line + ':' + err.column;
data = err.message.replace(/^ +/, '') + '\n\nin ' + fileLineColumn;
if (debug) log('error', 'error', '\x07\x1B[31m' + data + '\x1B[91m');

error(err);
var errorMessage = '\x07\x1B[31m' + err.message.replace(/^ +/, '') + '\n\nin ' + fileLineColumn + '\x1B[91m';

error(err, errorMessage);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved logging into error(); stopped overloading the data variable.

return next(err);
}

data = result.css;
var data = result.css;

if (debug) {
log('debug', 'render', options.response ? '<response>' : sassPath);
Expand Down Expand Up @@ -188,7 +187,7 @@ module.exports = function(options) {
res.end(data);
}

// If response is falsey, also write to file
// If response is true, do not write to file
if (options.response) {
return doneWriting();
}
Expand All @@ -198,12 +197,14 @@ module.exports = function(options) {

mkdirp(dirname(cssPath), '0700', function(err) {
if (err) {
return error(err);
error(err);
cssDone = true;
return doneWriting();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

as it was, in case of errors here, next() would never be called and the request would just hang there. We need to call doneWriting.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we add an option to toggle propagation to next middleware in case of error? My concern is someone might have taken dependency on this behavior by now and this would break them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is that nothing calls next() and nothing touches req after this point, so the request is left to time out.

Even if someone supplies options.error and we call them, they don't have access to req or next, so they can't do anything with the request.

In most other places, the call to error() is followed by next(), but not here; I can't imagine right now what dependency on this behaviour someone might have taken – can you please illustrate what you mean?

Propagating to the next middleware in case of error is the expected behaviour – we are a middleware after all.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense.

can you please illustrate what you mean?

For instance, some consumer might be reading the error and not existing as Sass middle ware is existing the pipeline for them.

With that being said, I don't have strong feelings about it. Consistent behavior wins. Thanks for elaboration. :)

}

fs.writeFile(cssPath, data, 'utf8', function(err) {
if (err) {
return error(err);
error(err);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as above

}

cssDone = true;
Expand All @@ -215,13 +216,16 @@ module.exports = function(options) {
var sourceMapPath = this.options.sourceMap;
mkdirp(dirname(sourceMapPath), '0700', function(err) {
if (err) {
return error(err);
error(err);
sourceMapDone = true;
return doneWriting();
}

fs.writeFile(sourceMapPath, result.map, 'utf8', function(err) {
if (err) {
return error(err);
error(err);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

both same as the above

}

sourceMapDone = true;
doneWriting();
});
Expand Down Expand Up @@ -265,18 +269,18 @@ module.exports = function(options) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's not really an error if we can't stat sassPath

Copy link
Collaborator

Choose a reason for hiding this comment

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

If sassPath doesn't exist then consumer of sass middleware must have wrong input path provider. Error/fail fast is the current behavior, this is intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I understand the code right, sassPath is computed from the request – it's the path to the potentially existing .scss file which, if it's there and younger than the .css file, will trigger compilation. Notice in the original code the error is logged with error(err) but then we just let next() handle the request.

Inside compile(), which gets called after this code, we check that sassPath fs.exists - that seems redundant now; or it indicates I don't quite get what's going on.

Copy link
Collaborator

Choose a reason for hiding this comment

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

that seems redundant now

Actually, this check is bit redundant because compile() has other callsites where we are not ensuring whether the file exists. We should error out (or log with debug severity) in compile function instead.

But lets keep it as is for now; I am planning to but some debug logs here and other places with new tag skip, to indicate that middleware recognized the request but since it can't fetch file stat, skipping to next. The issue #88 will ultimately get fixed once we also log a message and skip to next middleware when middleware receives a mismatching prefixed request (in case user has used the prefix option); so in the OP's example outdatedbrowser.min.css will not match the prefix, hence we skip to next middleware but still debug log that we (the middleware) saw the request and skipping to next due to such and such reasons.

This will help consumer debugging.

// Compare mtimes
fs.stat(sassPath, function(err, sassStats) {
if (err) {
error(err);
if (err) { // sassPath can't be accessed, nothing to compile
return next();
}

fs.stat(cssPath, function(err, cssStats) {
if (err) { // CSS has not been compiled, compile it!
if ('ENOENT' === err.code) {
if (debug) { log('error', 'not found', cssPath); }
if (err) {
if ('ENOENT' === err.code) { // CSS has not been compiled, compile it!
if (debug) { log('debug', 'not found', cssPath); }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not finding the CSS file is common, not really an error - it happens before the first compilation

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree the message can probably say: "not found , compiling .."

return compile();
}

error(err);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

give this error to options.error - that seems to be the expectation

return next(err);
}

Expand Down
42 changes: 35 additions & 7 deletions test/middleware.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,22 +33,34 @@ describe('Creating middleware', function() {

});

describe('Log messages', function() {
it('should use the default logger when none provided', function(done) {

var spawnedServer;

describe('Spawning example server', function() {
it('starts the server', function(done) {
var serverStartupTimeout = 950;
var expected = ' \u001b[90msource:\u001b[0m \u001b[36m' + index_scssFile + '\u001b[0m';
var bin = spawn('node', [fixture('example-server.js')]);
spawnedServer = spawn('node', [fixture('example-server.js')]);

// exclude serverStartupTimeout from timeout and slow counters of test runs
this.timeout(this.timeout() + serverStartupTimeout);
this.slow(this.slow() + serverStartupTimeout);

setTimeout(function() {
http.request({ method: 'GET', host: 'localhost', port: '8000', path: '/index.css' })
.end();
(spawnedServer.killed).should.be.false();
(spawnedServer.exitCode === null).should.be.true();
done();
}, serverStartupTimeout);
});
});

bin.stderr.once('data', function(data) {
describe('Log messages', function() {
it('should use the default logger when none provided', function(done) {
var expected = ' \u001b[90msource:\u001b[0m \u001b[36m' + index_scssFile + '\u001b[0m';

http.request({ method: 'GET', host: 'localhost', port: process.env.PORT || '8000', path: '/index.css' })
.end();

spawnedServer.stderr.once('data', function(data) {
data.toString().should.startWith(expected);
done();
});
Expand Down Expand Up @@ -578,3 +590,19 @@ describe('Checking for http headers', function() {
});

});

describe('Killing example server', function() {
it('stops the server', function(done) {
spawnedServer.kill();
var serverShutdownTimeout = 500;

// exclude serverStartupTimeout from timeout and slow counters of test runs
this.timeout(this.timeout() + serverShutdownTimeout);
this.slow(this.slow() + serverShutdownTimeout);

setTimeout(function() {
(spawnedServer.killed).should.be.true();
done();
}, serverShutdownTimeout);
});
});