Error handling #95

jacekkopecky · 2017-01-14T15:09:28Z

This addresses #93 and other issues found in course of fixing that. I'll add comments in the source about the various changes.

The changes in test/middleware.js are so that I could run the tests repeatedly – as it was, the test would start the example server but never kill it, so the next time tests run, EADDRINUSE would be thrown.

jacekkopecky · 2017-01-14T15:11:12Z

middleware.js

+
+      sassMiddlewareError = err;
+    };
+


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

added logging of all errors if debugging is on

jacekkopecky · 2017-01-14T15:11:42Z

middleware.js


+        error(err, errorMessage);


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

jacekkopecky · 2017-01-14T15:12:55Z

middleware.js

-          return error(err);
+          error(err);
+          cssDone = true;
+          return doneWriting();


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.

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.

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.

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. :)

jacekkopecky · 2017-01-14T15:13:01Z

middleware.js

        }

        fs.writeFile(cssPath, data, 'utf8', function(err) {
          if (err) {
-            return error(err);
+            error(err);


same as above

jacekkopecky · 2017-01-14T15:13:12Z

middleware.js

          }

          fs.writeFile(sourceMapPath, result.map, 'utf8', function(err) {
            if (err) {
-              return error(err);
+              error(err);


both same as the above

jacekkopecky · 2017-01-14T15:13:45Z

middleware.js

@@ -265,18 +269,18 @@ module.exports = function(options) {

    // Compare mtimes
    fs.stat(sassPath, function(err, sassStats) {
-      if (err) {
-        error(err);


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

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?

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.

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.

jacekkopecky · 2017-01-14T15:14:19Z

middleware.js

-            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); }


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

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

jacekkopecky · 2017-01-14T15:14:50Z

middleware.js

            return compile();
          }

+          error(err);


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

am11

Few comments / questions. Overall looks good. Thanks @jacekkopecky for doing this and especially for writing the tests! 👍

am11 · 2017-01-14T15:32:32Z

middleware.js

-          return error(err);
+          error(err);
+          cssDone = true;
+          return doneWriting();


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.

am11 · 2017-01-14T15:36:27Z

middleware.js

@@ -265,18 +269,18 @@ module.exports = function(options) {

    // Compare mtimes
    fs.stat(sassPath, function(err, sassStats) {
-      if (err) {
-        error(err);


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?

am11 · 2017-01-14T15:38:23Z

middleware.js

-            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); }


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

am11 · 2017-01-15T12:12:07Z

Thanks again @jacekkopecky ! 🎉

jacekkopecky added 2 commits January 13, 2017 17:26

spawn and kill the server in tests

c8d3262

error handling revamp

854f22e

jacekkopecky commented Jan 14, 2017

View reviewed changes

middleware.js

error(err, errorMessage);

Copy link

Contributor Author

jacekkopecky Jan 14, 2017

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.

jacekkopecky commented Jan 14, 2017

View reviewed changes

don't expect no exit code in the test stopping the server

d0ed31b

am11 reviewed Jan 14, 2017

View reviewed changes

am11 merged commit 3ef0775 into sass:master Jan 15, 2017

am11 mentioned this pull request Jan 15, 2017

sassMiddlewareError never emptied #93

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Error handling #95

Error handling #95

jacekkopecky commented Jan 14, 2017

jacekkopecky Jan 14, 2017

jacekkopecky Jan 14, 2017

jacekkopecky Jan 14, 2017

am11 Jan 14, 2017

jacekkopecky Jan 14, 2017

am11 Jan 14, 2017

jacekkopecky Jan 14, 2017

jacekkopecky Jan 14, 2017

jacekkopecky Jan 14, 2017

am11 Jan 14, 2017

jacekkopecky Jan 14, 2017

am11 Jan 14, 2017

jacekkopecky Jan 14, 2017

am11 Jan 14, 2017

jacekkopecky Jan 14, 2017

am11 left a comment

am11 Jan 14, 2017

am11 Jan 14, 2017

am11 Jan 14, 2017

am11 commented Jan 15, 2017

Error handling #95

Error handling #95

Conversation

jacekkopecky commented Jan 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

am11 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

am11 commented Jan 15, 2017