Generate 404.html page (#539)

[manuel-woelker]

Prototype spike for a 404 page (cf #539).

This PR generates a 404.html file with sidebar navigation and search intact, to better handle broken or stale links.
Additionally, the builtin server serves this file on 404 errors.

Comments and feedback appreciated!

[ehuss]

Thanks, looks useful!

Would it be possible to allow the user to customize the page? Like if src/404.md is found, use that? And maybe in the config have output.html.404 key that lets you set the filename if you don't want to use the default of src/404.md, or set that value to false to disable it altogether?

[manuel-woelker]

Thanks for the feedback and suggestions, I'll try to incorporate them over the next days.

[manuel-woelker]

Added the suggestions and rebased on the current master, which had become warped ;-)

HtmlConfig in [output.html] now has "input-404" (default: "404.md") and "output-404" keys (default: "404.html") to customize 404 generation.
404 generation can be disabled by setting output-404 = ""

[ehuss]

Thanks! Yea, sorry about swapping the webserver out, hopefully it wasn't too hard to figure out.

I noticed that it doesn't work for bad links in subdirectories. For example, visit http://localhost:3000/foo/bar and none of the css loads properly. I suspect it will need some HTML magic to get all the links to work properly.

Is there a particular reason to have separate configs for the input and output name? I suspect nearly everyone will just use the default, and the small number that don't can probably just set the input name and have that mirrored to the output. Also, if the output path has a different directory structure, the links won't work. I think I would prefer to keep the config simple, unless there is a particular use case for that.

[manuel-woelker]

Regarding subdirectories: I initially hoped this would be easy, alas the Content-Base and Content-Location headers that would have allowed these base URI to be set to fix this is no longer supported.

The other option is to set the URI in the html, which would either mean editing the index.html.hbs template, or selectively "fixing up" the 404 page to add it. The template approach would make relative URL resolution simpler, probably removing a lot of {{ path_to_root }} calls in the process. Any ideas about which approach is better here?

As for separate configs for the input and output name: I felt that it would be good for separation of concerns. The input path is a matter of repo layout - projects might want this file "out of the way" e.g. put it in a subdirectory or name it "_notfound.md". The output file is a "web serving" concern, i.e. where the web server expects the file. But I am open to changing that to make it simpler.

[ehuss]

Hm, that's a tough one. It seems like the only option would be to define a site-url in the config, and use absolute instead of relative paths. I took a look at another static site generator, and that's what it does. Links for normal pages are relative, but the 404 page uses absolute links relative to the site-url. For example, https://example.com/foo/bar/ would result in a link like /foo/bar/css/general.css in 404.html, and then the development server would override the site-url. That seems like a major change, and I'm uncertain what kind of fallout that would have, or how many places that needs to touch (maybe it is just path_to_root?).

[manuel-woelker]

Well, one option would be to only set the base url for all generated files to e.g. "/" using / in the head. Then all relative urls to css/js and other links are resolved relative to that base url, instead of the actual url path (AFAICT). This would simplify links for all pages, as well as fix the 404 in subdirectory issue. The only downside is that the site-url would have to be configured as an absolute path.

[ehuss]

I would prefer not to change the behavior for existing files. That would require knowing the site-url, and might break various deployment scenarios.

[manuel-woelker]

Hmm, I don't see any neat solution that allows path resolution from any URL without knowing the site url.

A hack would be to embed some javascript, which successively tries to "walk" the url upwards until it successfully downloads the css file, and with that information rewrites all the css and script includes in the DOM. This should work, but it's a bit of a kludge.

[ehuss]

I don't think it would be a problem for mdbook to require a site-url for the 404 page to work properly. If the site-url isn't set, the 404 page could generate absolute paths (/css/general.css), and emit a warning that site-url should be set for it to work properly.

[manuel-woelker]

Great, that sounds like a viable plan.

[manuel-woelker]

Added the config parameter output.html.site-url, and used that via the tag in the 404 page.

This also makes the navigation links work correctly.

[ehuss]

serve will need to modify the config to remove site-url (or maybe set it to /).

The documentation will need to be updated.

I would also still prefer to remove the output-404 and only have 1 option to configure it.

I'm also having doubts about whether or not it makes sense to have this part of the book, or part of the theme. I'm not really sure, just thought I'd toss it out there as an idea.

[ehuss]

This should probably be https://rust-lang.github.io/mdBook/, otherwise it would break on deployment.

[manuel-woelker]

Changed to "/mdBook/", the server should not be needed

[ehuss]

I'd like to avoid spamming all users to set the site-url. I'm not sure which is best, though. The 404 page could be disabled by default (with the assumption that sites usually have their own 404 mechanism), or it could just silently set site-url to / (or just lower this message to debug).

[manuel-woelker]

Changed to debug! now

[ehuss]

Can all this new code be moved to a separate function, so this function isn't so long?

[manuel-woelker]

refactored into its own function in impl HtmlHandlebars

[ehuss]

I think this will need to check the config for the name of the 404 page.

[ehuss]

I'm not sure why there are two different code blocks here. I think it can be done with a single one by using something like let filename = html_config.input_404.as_deref().unwrap_or("404.md");.

[manuel-woelker]

There's a case separation there: if the 404 input file is explicitly configured, but not found, that should a hard error. If it's not explicitly configured, a missing 404.md file should not error, but fall back to the default 404 message.

We could also remove the default behaviour, and make it an explicit opt-in.

[ehuss]

Instead of map_err, use with_context, something like this:

                std::fs::read_to_string(&path)
                    .with_context(|| format!("unable to open 404 input file {:?}", path))?

[manuel-woelker]

That reads much better, changed.

[ehuss]

Try to keep lines under 100 characters long. Break this up with backslashes.

Also, I'm not sure about the wording here. Maybe combine it with the example you wrote, maybe something like this:

# Document not found (404)

This URL is invalid, sorry. Please use the navigation bar or search to continue.

[manuel-woelker]

Updated default 404 wording and formatting.

[ehuss]

I'm not sure what "path" is used for, but I would guess this should be the path from the config?

[manuel-woelker]

AFAICT, that path is only used to create the path to root for navigation links and script urls, i.e. it should not be the config path in order for the links to work correctly.

[manuel-woelker]

Thanks for the thorough review and feedback!

I rebased and added the changes, as well as the documentation.

I removed the output-404 option as well.

Let me know if you have any further suggestions!

[ehuss]

Thanks!