search

jekyll / jekyll-feed

:memo: A Jekyll plugin to generate an Atom (RSS-like) feed of your Jekyll posts
MIT License
837 stars 203 forks source link

Use `relative_url` to generate feed path link. #317

Open fabacab opened 4 years ago

fabacab commented 4 years ago

Using the relative URL allows the plugin to be used without modification when generating sites that may be deployed across more than one domain name without needing to have the site regenerated for each domain name.

This commit also aligns the code with the code's comments (added in commit bf728c32) which say that the built-in Jekyll URLFilters are being used to provide the relative_url functionality, but then the code goes on to use the absolute_url filter.

Using the relative_url filter does not impact feed reader use (at least, in my testing), so this should be a backward-compatible change that improves the versatility of the rendered HTML code.

ashmaroli commented 4 years ago

also aligns the code with the code's comments

Perhaps the comment is incorrect. The previous Ruby code clearly takes config["url"] into account. Therefore, absolute_url is the correct choice.

fabacab commented 4 years ago

also aligns the code with the code's comments

Perhaps the comment is incorrect. The previous Ruby code clearly takes config["url"] into account. Therefore, absolute_url is the correct choice.

Yeah, that's possibly the original author's intent, but I think what matters more for correctness is whether or not a relative or an absolute URL in this specific context "works better" for the majority of situations jekyll-feed users use it in.

More specifically, is there a situation in which a relative URL does not work but an absolute_url does work? Put another way: assuming a relative URL produces the same behavior that an absolute URL does in all the situations where an absolute URL was intended, then given the fact that a relative URL also works in situations where an absolute URL does not work as I note in the commit message, I think the relative URL is actually the correct choice.

ashmaroli commented 4 years ago

At first glance, switching to Jekyll's relative_url seems like a safe move to me. (Prior to the released commit https://github.com/jekyll/jekyll-feed/commit/bf728c321d715f8bb1673e43fb3c848fe736fa6d, the plugin had also taken config["github"]["url"] into consideration)

That said, /cc @benbalter and @pathawks for further inputs.

pathawks commented 4 years ago

I see the value here. I wonder if this could be a breaking change: are we worried about sites that were capturing the <head> and replacing the feed link with a FeedBurner/CDN link? Maybe that is not a supported use case?

torrocus commented 4 years ago

Some older browsers have a problem with relative url in RSS/Atom feed. https://stackoverflow.com/questions/4438794/link-to-rss-atom-feed-relative-doesnt-work-in-firefox Can anyone confirm or deny this? Is the problem still current?

I also suggest looking at other, similar threads: https://github.com/gatsbyjs/gatsby/issues/14133 I understand that we do the opposite?

fabacab commented 4 years ago

For what it's worth, the W3C's Feed Validator Service correctly follows relative URLs to the location of a feed when you tell it to "Validate by URI" and provide the URL of an HTML page with a LINK element like:

<link type="application/atom+xml" rel="alternate" href="/feed.xml" title="Example Relative Feed" />

This is the expected (and standards-compliant) behavior for HTML4, HTML5, and all XHTML variants as far as I understand. Relative URLs are also explicitly mentioned as allowed on the MDN Wiki. My understanding is that this is also how feed readers that are not Web browsers should auto-discover a site's feed URL when given an HTML home page with such a LINK element in it.

Some older browsers have a problem with relative url in RSS/Atom feed. https://stackoverflow.com/questions/4438794/link-to-rss-atom-feed-relative-doesnt-work-in-firefox Can anyone confirm or deny this? Is the problem still current?

Firefox removed its built-in RSS/Atom feed support by 2018 so I'm not sure that Stackoverflow issue is relevant any longer.

torrocus commented 4 years ago

Firefox removed its built-in RSS/Atom feed support by 2018 so I'm not sure that Stackoverflow issue is relevant any longer.

It's good to know that Firefox no longer supports this.

I have another important question. Should this change not affect tests? If I understand correctly, different feed.xml files should be generated before and after the change.

ashmaroli commented 4 years ago

different feed.xml files should be generated before and after the change

Why would the feed be any different? The proposed change involves just the rendered <link ... /> tag..!!

torrocus commented 4 years ago

Ok, in this case, there is probably nothing to worry about.

DirtyF commented 4 years ago

Using the relative URL allows the plugin to be used without modification when generating sites that may be deployed across more than one domain

So it's theoretical and does not solve any real-word case scenario? Any example to provide where absolute is creating an issue?

fabacab commented 4 years ago

So it's theoretical

No, it's not theoretical, if you look at the codebase, you'll see it's really happening.

Any example to provide where absolute is creating an issue?

I currently use a patched version of jekyll-feed to cleanly generate HTML for static sites that can be run on the clearnet as well as a Tor Onion site with exactly one fewer modifications than the unpatched version of this code makes possible, but again, if you take a minute to more carefully read the original PR this would be self-evident to you.

DirtyF commented 4 years ago

I did read, please understand as maintainers we have to put things in perspective:

  1. We already generate a valid URL feed (with absolute URL)
  2. On the 567 508 users of the gem, only 1 person is asking for this
  3. You already patched it for your edge case.

I would mark it as won't fix. I'll defer to @pathawks and @ashmaroli on this one.

fabacab commented 4 years ago

I did read, please understand as maintainers we have to put things in perspective

I understand maintainers are busy. That's why I upstreamed my patch as a PR, so you wouldn't have to do any extra work.

On a non-technical note, please understand it's a bit irksome and very off-putting to be gruffly approached as though I'm asking for something theoretically when it's plain as day by prior conversation that that isn't the case, especially in the context of having gone out of my way and taken the extra time to offer to contribute to this project. You could have just posted, "Can you give us a specific example where this modification is useful to you?" You'd have ellicited a less obviously irritated response from me, and simultaneously saved yourself some typing.

I would mark it as won't fix. I'll defer to @pathawks and @ashmaroli on this one.

I'll reiterate what I wrote in my original PR, emphasis added:

Using the relative_url filter does not impact feed reader use (at least, in my testing), so this should be a backward-compatible change that improves the versatility of the rendered HTML code.

Generating sites for Tor Onion services are not the only scenario in which relative URLs are helpful, and I don't even expect it to be the most common one, it just happens to be the reason I patched the Gem for myself. If there is a situation in which an absolute URL "works" but a relative URL does not, it hasn't been brought up on this thread yet. But the inverse, cases where a relative URL "works" and an absolute one does not, have.