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

HTML Pipeline filter to make all relative urls absolute #12

Closed
wants to merge 6 commits into from

Conversation

bkeepers
Copy link

I'm struggling to come up with a way to make content for opensource.guide render on both GitHub and the pages site. Current solutions require using Jekyll filters (![][{{ "url" | relative_url }}]), which breaks rendering on GitHub (and breaks our pedantic markdown tests since that is not valid markdown).

Here's a suggestion for an alternative approach: all "absolute" urls are assumed to be absolute to the site root, and are automatically corrected during rendering. This PR adds an HTML pipeline filter (largely borrowed from the filter used for rendering repository content on github.com) that makes urls for links and images relative to the base url.

  • <a href="/thing">link</a> => <a href="/baseurl/thing">link</a>
  • <a href="thing">link</a> => <a href="/baseurl/currentpage/thing">link</a>
  • <img src="/thing.png"> => <img src="/baseurl/thing.png">

These URLs are unaffected:

  • Explicit protocol, e.g. https://example.com/
  • Protocol relative, e.g. //example.com/
  • Hash, e.g #foobar
  • Already includes base url, e.g. /baseurl/foo

You can see progress on getting this working here: github/opensource.guide#315.

If this works, I think would remove the need for relative_url and absolute_url filters.

@benbalter @parkr - what do you think about this approach?

cc github/opensource.guide#307

@benbalter
Copy link
Owner

@bkeepers I'm having trouble following what's going on in github/opensource.guide#315 and github/opensource.guide#307.

Before we jump to the solution, can you share a bit more context around what you're trying to accomplish, what you tried, what's not working, etc.?

@bkeepers
Copy link
Author

Before we jump to the solution, can you share a bit more context around what you're trying to accomplish, what you tried, what's not working, etc.?

I want all of the links and images in markdown to work, whether it's rendered on GitHub Pages or GitHub.com.

Given this directory layout:

  • foo.md
  • images/
    • benbalter-in-a-prodigy-shirt.jpg

Scenario 1:

foo.md has this content:

![@benbalter looking fine](images/benbalter-in-a-prodigy-shirt.jpg)

This works fine on GitHub.com, because the images directory is relative to foo.md.
When foo.md is rendered with a permalink of /foo/, it's now looking for /foo/images/.

Scenario 2:

Ok, we can fix that by switching the relative image in foo.md to an absolute url:

![@benbalter looking fine](/images/benbalter-in-a-prodigy-shirt.jpg)

This works fine on GitHub.com, because it uses a filter similar to this one that makes all absolute urls relative to the repository, and it works fine when served from jekyll without a baseurl. But when you add a baseurl (e.g. someone forks your site and renders it on theirusername.github.io/project-name), the absolute URLs now break.

The solution

A jekyll site currently has to do a lot of gymnastics with the relative_url liquid filter to get links and images to work, which breaks the content on GitHub.com. This plugin already attempts to solve that by converting links to markdown files to their permalink alternatives (although it doesn't work for files in collections, which I'll submit a fix for after this PR is resolved)

The filter added in this PR would make it so content and templates use absolute urls everywhere and it always works, whether it's rendered on GitHub.com, Pages, or a Pages site with a baseurl. It also has the advantage of making links in templates work the same way, so theme authors can use absolute URLs everywhere without having to use liquid filters.

@benbalter
Copy link
Owner

it's now looking for /foo/images/.

It shouldn't be doing that. To preserve the behavior on .com, it should look to resolve the link path relative to the file path, not relative to the published permalink URL. It should also be able to traverse to parent directories, as you describe. See https://github.com/benbalter/jekyll-relative-links/blob/master/spec/jekyll-relative-links/generator_spec.rb#L57-L64.

A jekyll site currently has to do a lot of gymnastics with the relative_url liquid filter to get links and images to work, which breaks the content on GitHub.com.

This plugin was designed to prevent that. You shouldn't be putting filters in your otherwise-vanilla markdown URLs, and in fact uses the relative_url filter internally to prepend baseurls as necessary.

What I think the issue might be is that this plugin currently only supports links to Markdown files, not images.

Based on https://help.github.com/articles/about-readmes/#relative-links-and-image-paths-in-readme-files, it sounds like it needs to support images as well if we want feature parity.

That leaves me with two questions:

  1. Can you reproduce the above with Markdown files, rather than image?
  2. If the above behavior was implemented for images, would that resolve the problem?

@bkeepers
Copy link
Author

Can you reproduce the above with Markdown files, rather than image?

I think that part works fine (well, after #13).

If the above behavior was implemented for images, would that resolve the problem?

If #13 is merged, I think it solves it for markdown links and images.

This implementation has the advantages of also working with script, link, and anything used in templates. You could tell Jekyll users to use absolute URLs and everything will always work everywhere, period.

My turn to ask questions:

  1. Can you think of any situation where this implementation will break?
  2. While you could argue that this implementation is counter to people's understanding of how hyperlinks in markup work, is it any more counter than the current situation with Jekyll of having to use the exact filename in .md files, and use the relative_url for everything else?

@bkeepers
Copy link
Author

bkeepers commented Mar 1, 2017

@benbalter any thoughts on this? Images are broken on the Open Source Guides for any fork (e.g.) and I'd love to get this resolved. I'll start exploring other options if this isn't a viable option.

@benbalter
Copy link
Owner

@bkeepers I think this best route forward would be to:

  1. Add collection support to this plugin (ideally opt-in)
  2. Add image support to this plugin using the existing logic

I'm hesitant to go the HTML pipeline route, especially for something that's enabled by default.

  1. Given the size of GitHub Pages, we've had issues with Nokigiri parsing documents causing the build to fail (unclosed tags, non-UTF-8, non-standard markup, etc). Not to mention, when it does work, Nokigiri seems to modify the output. It strips google custom search, and adds its own meta tags without users' consent.

  2. I'm not sure that processing the output will achieve the result you're looking for. Let's say I have a.md with permalink /a/ and b.md with permalink /a/b/. In GitHub.com, I'd link from A to B with a.md. On GitHub.com it'd link you to a.md's blob view, and with this plugin, it should resolve b.md to /b/ and link you to /a/b/index.html. If we looked at the outputted HTML, how would we resolve permalinks when A or B have different permalinks than their file path (and need to compute their output extension).

I'll try to queue up time to tackle image support, but am glad to review a PR to add support (and get things bumped) if you are able to before I can.

@bkeepers
Copy link
Author

bkeepers commented Apr 8, 2017

Closing based on feedback. Thanks.

@bkeepers bkeepers closed this Apr 8, 2017
@bkeepers bkeepers deleted the fix-all-the-urls branch April 8, 2017 00:08
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.

2 participants