search

edgecase / dieter

Asset pipeline ring middleware
134 stars 22 forks source link

Use fs 1.3.2; Throw coffeescript compile errors in browser #30

Closed timcharper closed 11 years ago

timcharper commented 11 years ago

This is a combo-pull request. Sorry about that.

The first commit upgrades dieter to use fs 1.3.2, as 0.11 was heavily outdated and, naturally, very incompatible with 1.3.2

The second commit causes CoffeeScript compile errors to surface to the browser. Rather than rendering an empty page with 500 status, it renders the compile error as a throw('') script, so you immediately see javascript error in the browsers console, plus the details about which file failed to compile, which line number, and the CoffeeScript generated reason.

timcharper commented 11 years ago

I've added a 3rd commit, which causes less compile errors to surface to the page via the css content attribute.

pbiggar commented 11 years ago

The fs change is great. I strongly do not like the other changes.

It seems to me that having errors be errors is good, as is reporting them as early as possible.

I know it's annoying to try to find the error in the stack trace in noir, but that could be solved in different ways.

I believe this prevents compile errors from throwing 500s, causing errors to be misreported as successes in logs, error trackers, and chrome/firefox tooling. Am I wrong?

timcharper commented 11 years ago

On Sep 23, 2012, at 10:06, Paul Biggar notifications@github.com wrote:

The fs change is great. I strongly do not like the other changes.

It seems to me that having errors be errors is good, as is reporting them as early as possible.

I know it's annoying to try to find the error in the stack trace in noir, but that could be solved in different ways.

Hmm, I'm using dieter with Compojure, and was not seeing the errors reported anywhere... hence my motivation.

I believe this prevents compile errors from throwing 500s, causing errors to be misreported as successes in logs, error trackers, and chrome/firefox tooling. Am I wrong?

I see a really good point about the request should return exit-status 500. If it did use exit-status 500, what would be the harm in outputting the error to the browser?

What are some other ways you think you might solve this? What if it pushed out a 500 page, with the content containing the error message? And if we did that, would there be any reason to format it in such a way that it would be visible to the browser?

pbiggar commented 11 years ago

If it did use exit-status 500, what would be the harm in outputting the error to the browser?

I'm not sure the JS/CSS would be used on a 500? Not 100% sure. Having the error appear to the user would not be a good thing though, to do by default.

It seems like it's something compojure should fix? How do server-side errors normally bubble up?

pbiggar commented 11 years ago

Oh, and it occurs to me that this might break lein dieter-precompile, which reports exceptions.

timcharper commented 11 years ago

On Sep 23, 2012, at 11:32, Paul Biggar notifications@github.com wrote:

I'm not sure the JS/CSS would be used on a 500? Not 100% sure. Having the error appear to the user would not be a good thing though, to do by default.

I'm not sure if you meant to include the word not in "Having the error appear to the user would not be a good thing"… as in, you're saying, it's best to render an empty 500 response, and would not be a good idea to return content-type text with the associate error message as the content?

timcharper commented 11 years ago

Oh, and it occurs to me that this might break lein dieter-precompile, which reports exceptions.

Very good point. So in the end, whether you agree to have the resulting 500 error pages empty or contain the error message description, we should ultimately have it throw an error message such that dieter-precompile catches it.

pbiggar commented 11 years ago

I'm saying that having it appear to the user is not a good idea :) That is, it's not a decision a library should make for a website that uses it.

In fact, that might be the principle I'm applying in general here: dieter is a library which should behave "normally" and in a composable way. So injecting JS into a site to throw an error is not usual or composable, nor is the CSS change. If there were a way to provide this information such that we could opt-in to using it on the client, then that would be OK. But it feels like this logic belongs in the web server, not in dieter.

pbiggar commented 11 years ago

I'm going to close this - would be good to get the fs changes in, but we can do that in a different issue. I'm sympathetic to the changes here, it just seems that dieter is the wrong part of the stack to do it.