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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

feat(gatsby-plugin-feed): add feedUrl plugin option #13826

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
47fdedc
feat(gatsby-plugin-feed): add feedUrl plugin option
leonardodino May 3, 2019
614e3c7
test(gatsby-plugin-feed): add feedUrl to pathPrefix e2e tests
leonardodino May 3, 2019
c1edbaa
test(gatsby-plugin-feed): stricter url checking in e2e test
leonardodino May 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion e2e-tests/path-prefix/cypress/integration/asset-prefix.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,12 @@ describe(`assetPrefix`, () => {

describe(`gatsby-plugin-feed`, () => {
it(`prefixes RSS feed`, () => {
assetPrefixMatcher(cy.get(`head link[type="application/rss+xml"]`))
assetPrefixMatcher(cy.get(`head link[type="application/rss+xml"]:first`))
})
it(`keeps RSS feedUrl intact`, () => {
Copy link
Contributor

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!

cy.get(`head link[type="application/rss+xml"]:last`)
.should(`have.attr`, `href`, `http://localhost:9000/rss-2.xml`)
.and(`not.matches`, assetPrefixExpression)
})
})
})
23 changes: 23 additions & 0 deletions e2e-tests/path-prefix/gatsby-config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,29 @@ module.exports = {
title: `assetPrefix + pathPrefix RSS Feed`,
output: `rss.xml`,
},
{
query: `
{
pages: allSitePage {
nodes {
path
}
}
}
`,
serialize({ query: { site, pages } }) {
return pages.nodes.map(node => {
return {
description: `A sample page hello world suh dude`,
date: `10-08-1990`,
url: `${site.siteMetadata.siteUrl}${pathPrefix}${node.path}`,
}
})
},
title: `feedUrl RSS Feed`,
feedUrl: `http://localhost:9000/rss-2.xml`,
Copy link
Contributor

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?

Copy link
Contributor Author

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)

Copy link
Contributor

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?

Copy link
Contributor Author

@leonardodino leonardodino May 3, 2019 •
edited
Loading

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)

Copy link
Contributor

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.

Copy link
Contributor

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!)

Copy link
Contributor Author

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

output: `rss-2.xml`,
},
],
},
},
Expand Down
27 changes: 27 additions & 0 deletions packages/gatsby-plugin-feed/src/__tests__/__snapshots__/gatsby-ssr.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,32 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`Adds <Link> for feed to head creates Link href with feedUrl, ignoring __PATH_PREFIX__ 1`] = `
[MockFunction] {
"calls": Array [
Array [
Array [
<link
href="https://example.org/gryffindor/feed.xml"
rel="alternate"
type="application/rss+xml"
/>,
<link
href="https://example.org/ravenclaw/feed.xml"
rel="alternate"
type="application/rss+xml"
/>,
],
],
],
"results": Array [
Object {
"type": "return",
"value": undefined,
},
],
}
`;

exports[`Adds <Link> for feed to head creates Link href with path prefix when __PATH_PREFIX__ sets 1`] = `
[MockFunction] {
"calls": Array [
Expand Down
26 changes: 26 additions & 0 deletions packages/gatsby-plugin-feed/src/__tests__/gatsby-ssr.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,33 @@ describe(`Adds <Link> for feed to head`, () => {
expect(setHeadComponents).toMatchSnapshot()
expect(setHeadComponents).toHaveBeenCalledTimes(1)
})
it(`creates Link href with feedUrl, ignoring __PATH_PREFIX__`, async () => {
global.__PATH_PREFIX__ = `/hogwarts`

const pluginOptions = {
feeds: [
{
output: `/gryffindor/feed.xml`,
feedUrl: `https://example.org/gryffindor/feed.xml`,
},
{
output: `/ravenclaw/feed.xml`,
feedUrl: `https://example.org/ravenclaw/feed.xml`,
},
],
}
const setHeadComponents = jest.fn()

await onRenderBody(
{
setHeadComponents,
},
pluginOptions
)

expect(setHeadComponents).toMatchSnapshot()
expect(setHeadComponents).toHaveBeenCalledTimes(1)
})
it(`creates Link with a title if it does exist`, async () => {
const pluginOptions = {
feeds: [
Expand Down
4 changes: 2 additions & 2 deletions packages/gatsby-plugin-feed/src/gatsby-ssr.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ exports.onRenderBody = ({ setHeadComponents }, pluginOptions) => {
...pluginOptions,
}

const links = feeds.map(({ output, title }, i) => {
const links = feeds.map(({ feedUrl, output, title }, i) => {
if (output.charAt(0) !== `/`) {
output = `/` + output
}
Expand All @@ -22,7 +22,7 @@ exports.onRenderBody = ({ setHeadComponents }, pluginOptions) => {
rel="alternate"
type="application/rss+xml"
title={title}
href={withPrefix(output)}
href={feedUrl || withPrefix(output)}
/>
)
})
Expand Down
1 change: 1 addition & 0 deletions packages/gatsby-plugin-feed/src/plugin-options.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import Joi from "joi"
// TODO: make serialize required in next major version bump
const feed = Joi.object({
output: Joi.string().required(),
feedUrl: Joi.string(),
query: Joi.string().required(),
title: Joi.string(),
serialize: Joi.func(),
Expand Down