krakenjs / kraken-devtools

Development-time tools for kraken.js applications.
Other
40 stars 32 forks source link

Middleware errors are not output to console #41

Open bryanspears opened 9 years ago

bryanspears commented 9 years ago

I noticed that LESS compile errors aren't being output to the console in development mode. I tried it on a fresh kraken app to verify it wasn't my app. I can see the error on the 500 page if I visit the css path directly.

For dust/i18n/browserify I get an "uncaughtException Cannot find module './'" error when any syntax errors occur which isn't very helpful.

Obviously, if I find the time in the near future to dig into why and submit a PR, I will. :smile:

bryanspears commented 9 years ago

Thought about it a bit more: why don't dev and prod modes handle errors using the same code path? Shouldn't both cause a 500 page? Overall, error handling in the kraken setup already feels a bit incoherent, and it makes testing feel strange when you get different results based simply on dev vs. prod modes.

pvenkatakrishnan commented 9 years ago

Hey @bryanspears it depends on how you set up your error middleware. if you just have the same for both prod and dev, the same middleware will get used. Can you share a project/ code that demonstrates otherwise so that i can understand the issue better

bryanspears commented 9 years ago

This is specifically a dev mode issue with devtools.

If there's an error in your less code, I often have to load the resulting resource directly (e.g. /css/app.css) to see the exact error. The less compiler (or other devtools plugins) errors do not show in the main (node) console.

To reproduce: Use the kraken generator to create a brand new kraken app. Type invalid Less into app.less. Run the app and load index. There's no error in the console for the Less compile failure, but if you load localhost:8000/css/app.css you'll then see your error.

grawk commented 9 years ago

@bryanspears what about an additional debug log message here, to print out any errors: https://github.com/krakenjs/kraken-devtools/blob/master/lib/middleware.js#L83

if (!!err) {
  debuglog('Encountered error: ', err);
}

For less, you would get the "production" error at the time of grunt build and not at the time of a request for the css resource.

grawk commented 9 years ago

A wise teammate of mine pointed out that the 500 middleware does intercept this error. You can currently see the error by setting NODE_DEBUG=kraken/middleware/500

bryanspears commented 9 years ago

I understood the reasoning, it's a dev only concern, but I don't see why seeing "all errors" in devote isn't a default or very easy to find option.

Is there such an top-level option to see all the errors?

I appreciate the help!

grawk commented 9 years ago

Initially I'd tried to see all the logging of all kraken* modules by using NODE_DEBUG=kraken*

Didn't seem to work but I haven't yet read the debuglog doc.

jasisk commented 9 years ago

Hey Bryan,

You're not seeing the errors because of the unfortunate 5xx middleware that is wired up automatically by the generator.

If you remove it from your config, you'll see error messages in your console again.

grawk commented 9 years ago

Or I guess override it in your development.json

bryanspears commented 9 years ago

Jean-Charles hit it on the head. I have my own 500/error handling middleware to take over logging and such, but it didn't handle the middleware errors correctly.

I still believe that "if in dev mode, show ALL errors" is how every app stack should operate unless configured otherwise. I shouldn't have to hunt down errors in dev mode, right?

Thanks everyone! Feel free to close this issue if the team disagrees with my opinion. :smile:

grawk commented 9 years ago

I think it would be nice if all the logging could be turned on easily. Apparently debuglog doesn't work the way I thought it should. I've been using https://github.com/visionmedia/debug in my own modules and it allows wildcards. Would be nice if all the kraken logging could be turned on easily e.g. NODE_DEBUG=kraken*

grawk commented 9 years ago

Passing through again, trying to resolve issues of old. @bryanspears and others.. Do you think the error should be thrown in the less plugin itself?

grawk commented 9 years ago

Pardon that... Let's not say we are throwing any errors. But based on Bryan's earlier comment about a "top-level option to see all the errors" I would propose that we add such an option to the configuration of devtools. This would toggle on/off a console.error statement including any error from any registered plugin.

bryanspears commented 9 years ago

Since this is a module explicitly created for developers and only enabled in development mode. Why not have all errors dumped to console by default? Obviously, if there's more plumbing to that, then there's deeper discussions to be had.

grawk commented 9 years ago

It would essentially be a one line change to add console.error here: https://github.com/krakenjs/kraken-devtools/blob/master/index.js#L50

To add a conditional around that and make it configurable would be a 3 line change... Or still one line depending on how clever one wants to be.