-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
feat(gatsby-plugin-feed): add feedUrl plugin option #13826
Conversation
ensure it's not rewriting any part of the feedUrl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding this! Few notes/questions:
- Thanks for adding all the tests--makes this a lot easier to review!
- Isn't the generated url already stable? I don't see what this is doing exactly, apart from making it so that assetPrefix isn't really being respected for this asset?
}) | ||
}, | ||
title: `feedUrl RSS Feed`, | ||
feedUrl: `http://localhost:9000/rss-2.xml`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So in this case, output isn't used whatsoever?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
output
is still used for saving the file to disk (gatsby-node.onPostBuild
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For clarity/conciseness then, it'd probably be a good idea to not include the output file in feedUrl, and infer it from the existing output?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that actually fits my use case:
currently my feedUrl is https://example.org/${output}
.
I left intentionally as a escape-hatch for even more complex cases, where an user can opt-out of the magic.
I can rewrite this portion if needed (: (but I think we should change it to something like baseUrl
as it won't be the final one in the DOM)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think siteUrl
is typically used for this--right? i.e. siteMetadata.siteUrl
refers to the deployed URL of your application.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(not suggesting you change the name--just an observation!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, siteUrl is set to https://example.org/
, I looked into using it directly, and only having a boolean for using it as a absolute path, but it seems I can't query it inside of the gatsby-ssr.onRenderBody
assetPrefixMatcher(cy.get(`head link[type="application/rss+xml"]`)) | ||
assetPrefixMatcher(cy.get(`head link[type="application/rss+xml"]:first`)) | ||
}) | ||
it(`keeps RSS feedUrl intact`, () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Thanks for adding these!
Sorry if it wasn't clear, more precisely, I'm using netlify More precisely: |
@leonardodino it seems like there'd be better ways to remediate outside of adding in additional options to the plugin, however. Could you share more detail on your deploy, or even a link to the repository, itself? As I understand the problem, you're using Netlify's |
Hey @DSchau I will have to consult whether people feel ok with sharing the complete example. I agree it's better to not over-complicate core plugins. We are using some complex netlify rules, serving many "smaller" sites stitched together, the total build time for everything under the Due to limitations on netlify's redirect system, especially regarding nested redirects and caching, this was the solution we found, to have files linked on another domain, instead of relying on the Just having the option to not generate the Right now I have to keep using an earlier version, being careful not to upgrade. |
Hi @leonardodino 👋 Did you find the time to build an example/collect details on your deploy? Currently I also feel like this is an edge case / quite don't understand the use case unfortunately :/ |
Hey @LekoArts, thanks for triaging this issue! (: Unfortunately I don't have much more info, besides what I said in the last comment. I pinged the team to see if there's any more details behind the explanation. I believe my case is a bit more general than just a feature for the re-reading one of the @DSchau questions:
I figured out that a feed is not actually an asset, it's a user-visible URL. Example: when compressing images we don't care that much about the URL, it will be cited in the HTML for the browser to fetch, it can be long and random. |
UPDATE: You may close this PR now. I've "hacked" the (only saw that other PR today, it happens that we both opened them only hours apart.) |
@leonardodino How about closing this PR (to keep the history of the changes clean) and you could open up a new PR to document your usage in the README? |
Thanks so much for the work on the
asset-prefix
PR! 🎉I just found a small setback, this PR solves that.
Description
use case:
My project uses a different url for each build (via the just-released
--path-prefix
option), but the feedUrl should be kept stable, as it's user visible.The built urls are behind a reverse-proxy, and only appear for stable/hashed resourced. For feed readers, the url must continue the same.
Related Issues
Related to #12128