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.

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

use roots and support non-404 errors #5

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

use roots and support non-404 errors #5

wants to merge 5 commits into from

Conversation

notslang
Copy link

@notslang notslang commented Jun 3, 2014

closes #4

also allow errors to be sent as JSON, HTML and plain text depending on
the accepts header
one is all you need
@jescalan
Copy link

jescalan commented Jun 3, 2014

Any reason we need roots for such a simple page here?

@notslang
Copy link
Author

notslang commented Jun 3, 2014

roots is for 2 reasons:

@kylemac
Copy link
Contributor

kylemac commented Jun 4, 2014

hmm I don't think that I like having the roots project inside apology. for one, it's making the dependency much bigger than it has to. it also may be a bit confusing for a contributor not familiar with roots.

My thinking is that it's totally fine to use roots to build the error pages, but let's take that out of the middleware itself. if you really think it's necessary, maybe make a different roots project that builds the different pages, but let's only include the compiled pages in the middleware.

Also, I'd like for the pages to be inline so that they are self contained (meaning no external assets like .css or .js files, or images).

@notslang
Copy link
Author

notslang commented Jun 4, 2014

All of the roots stuff is in devDeps, so it won't get installed when you're using apology inside of charge. Also, we can ignore all of the compiled languages when we deploy to npm using .npmignore. The result will be like roots isn't even there.

As for inlining, take a look at https://github.com/carrot/apology-middleware/blob/slang/public/index.html. Everything is compiled into one self-contained file (without having them all mashed together while developing).

everything is compiled into ./public, so those aren't needed
@kylemac
Copy link
Contributor

kylemac commented Jun 4, 2014

ah, I definitely misread the diff and thought they were regular deps. yeah - that works for me then - thanks for the clarification @slang800.

@coveralls
Copy link

Coverage Status

Coverage decreased (-33.33%) when pulling b73c5db on slang into 75eb080 on master.

@joshrowley
Copy link

Just my two cents, using roots for this seems unnecessary for the current state of the project. Fine if you're planning to use it to add more powerful features, but I wouldn't add this until that need arises. Using roots here adds another tool a potential contributor/user will need to be familiar with.

It just seems like adding more complexity for no significant benefits right now.

@jescalan
Copy link

jescalan commented Jun 5, 2014

I agree with @joshrowley and @kylemac here

@notslang
Copy link
Author

notslang commented Jun 5, 2014

Is using roots really that hard? I felt like it is easier than most build tools, but I suppose I'm biased because I've been using it for quite some time.

And @jeff - you mean kyle's earlier comment before he said it's ok? Agreeing with both would take some doublethink ; )

@joshrowley
Copy link

@slang800 It's not that roots is hard to use, it's that most people have never used it. This just adds more cognitive overhead to this project. A large majority developers will immediately grok an html file. This adds a much more complex directory structure, not to mention jade and stylus files (which people might not be familiar with). And for what value in return?

This is also in anticipation for "powerful" features that have not been implemented yet. At the very least, PR something that really takes advantage of using roots along with this, but at the moment I really don't think this an appropriate use case for roots.

@jescalan
Copy link

jescalan commented Jun 6, 2014

I'm about as biased as it comes since I work on roots full time, but I still don't think it's appropriate here haha.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error page is broken
5 participants