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

feat: generate rss feed #364

Merged
merged 20 commits into from
Apr 16, 2020
Merged

feat: generate rss feed #364

merged 20 commits into from
Apr 16, 2020

Conversation

davifantasia
Copy link
Contributor

Closes #361

@vercel
Copy link

vercel bot commented Apr 14, 2020

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/commercetools/commercetools-docs-kit/7j98g6su9
✅ Preview: https://commercetools-docs-kit-git-rss-feed.commercetools.now.sh

@davifantasia davifantasia added the 🚀 Type: New Feature Something new label Apr 14, 2020
@vercel vercel bot temporarily deployed to Preview April 14, 2020 14:37 Inactive
betaLink: null,
siteUrl: `https://${productionHostname}/${pluginOptions.websiteKey}`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: the websiteKey is not a guarantee that it's the same as the URL path.

What you're looking for is the pathPrefix I think.

@@ -210,6 +213,54 @@ module.exports = (themeOptions = {}) => {
'gatsby-plugin-remove-trailing-slashes',
'gatsby-plugin-meta-redirect',
'gatsby-plugin-netlify-cache',
{
resolve: 'gatsby-plugin-feed',

This comment was marked as resolved.

This comment was marked as resolved.

Copy link
Collaborator

@nkuehn nkuehn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bunch of comments concerning improving the content of the resulting feed.

See https://www.npmjs.com/package/rss#feedoptions for more precise descriptions than the GatsbyJS documentation example

@@ -210,6 +213,54 @@ module.exports = (themeOptions = {}) => {
'gatsby-plugin-remove-trailing-slashes',
'gatsby-plugin-meta-redirect',
'gatsby-plugin-netlify-cache',
{
resolve: 'gatsby-plugin-feed',

This comment was marked as resolved.

query: `
{
allReleaseNotePage(
sort: { order: DESC, fields: date },
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to filter by a cutoff date to be able to reduce the feed to new entries only during migration. Or just live with it that users will get five old release notes again if they use a feed reader

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feeds should not contain the full history, only as many as a user initially subscribing the feed should immediately get in their reader UI and the longest period we imagine some client being offline to not check for updates.

We need to limit the number of entries to a reasonable amount. Currently we have five, which did the job in the past.

) {
nodes {
description
body
Copy link
Collaborator

@nkuehn nkuehn Apr 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don't find a solution for the encoding (see custom_elements) this field does not need to be queried any more.

...node,
url: site.siteMetadata.siteUrl + node.slug,
guid: site.siteMetadata.siteUrl + node.slug,
custom_elements: [{ 'content:encoded': node.body }],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MDX is not HTML, you can't just trow MDX into an RSS as HTML encoded content.

I don't know if it's realistic to get a usable plaintext or "minimal HTML" representation directly from the GraphQL for our (sometimes custom) MDX, so I wold be fine omitting it as we currently do, too.

But we can try - one option that publishers take is to put the excerpt until "read more" here so people move on to your actual website instead of staying in some feed consumer UI. But even then it has to be either plaintext or plain semantic HTML

}
`,
output: '/release-notes.xml',
title: `${pluginOptions.websiteKey}'s rss feed`,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's try a better name :-)

pseudocode proposal: commercetools ${siteMetadata.title} Release Notes

@vercel vercel bot temporarily deployed to Preview April 15, 2020 09:28 Inactive
@davifantasia davifantasia marked this pull request as ready for review April 15, 2020 09:31
@vercel vercel bot temporarily deployed to Preview April 15, 2020 16:04 Inactive
@nkuehn nkuehn self-requested a review April 15, 2020 19:30
@vercel vercel bot temporarily deployed to Preview April 15, 2020 19:30 Inactive
nkuehn
nkuehn previously requested changes Apr 15, 2020
Copy link
Collaborator

@nkuehn nkuehn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@davifantasia I did two small changes (see comments). omitting the full text is good, all the hacks we considered are just bad content.

Two things remain to be done:

  • only load and run the plugin when the theme option "hasReleaseNotes" is true.
  • move the plugin before the ones that are explicitly tagged as having to be the last ones in the list.

slug
title
date
categories: topics
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this as a quick win feature

@@ -267,7 +267,7 @@ function generateReleaseNoteSlug(node) {
const date = node.frontmatter.date ? node.frontmatter.date.split('T')[0] : '';
const title = node.frontmatter.title ? node.frontmatter.title : '';

const slug = slugify(`${date} ${title}`, { lower: true });
const slug = slugify(`${date} ${title}`, { lower: true, strict: true });
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did add "strict" to the slugify to make sure e.g. not parantheses and other characters are in the URL to have them consistent with all existing URLs.

}
}
`,
output: '/release-notes.xml',

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

@emmenko

This comment has been minimized.

@nkuehn

This comment has been minimized.

@nkuehn nkuehn dismissed their stale review April 15, 2020 19:54

obsolete

@vercel vercel bot temporarily deployed to Preview April 15, 2020 19:59 Inactive
@vercel vercel bot temporarily deployed to Preview April 15, 2020 20:04 Inactive
Copy link
Collaborator

@nkuehn nkuehn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I implemented the open topics, good to go now from my side

@vercel vercel bot temporarily deployed to Preview April 16, 2020 09:22 Inactive
@@ -12,9 +12,9 @@ module.exports = {
{
resolve: '@commercetools-docs/gatsby-theme-docs',
options: {
title: 'Docs Smoke Test',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also duplicated

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, not quite sure what the alternative would look like yet.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could "move" the siteMetadata as a theme plugin option, then spread the value in the theme gatsby-config.js as we have some defaults for siteMetadata.

This would be a breaking change though.

Copy link
Contributor Author

@davifantasia davifantasia Apr 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm looking to override the title in the setup field of the plugin, where we have access to siteMetadata.

https://www.gatsbyjs.org/docs/adding-an-rss-feed/#syntax-for-itunes-rss-blocks

But I see that the feed's global title is the same as the description, I'll try to fix that but it seems it's just a workaround as this behaviour is probably a bug in the plugin.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@emmenko we just refactored the way the options are read, no duplicates any more

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I like the setup approach. The configuration has still an error though (see CI)

…iption and link of the channel are generated
@vercel vercel bot temporarily deployed to Preview April 16, 2020 10:49 Inactive
}
}
`,
setup: (options) => {
Copy link
Contributor Author

@davifantasia davifantasia Apr 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can override the site title here to customize with title from siteMetadata.

But the link and description of the channel in the generated xml are lost once setup is implemented. Even when I tested with just returning the option parameter.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vercel vercel bot temporarily deployed to Preview April 16, 2020 11:46 Inactive
Copy link
Member

@emmenko emmenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the default implementation, we should also spread siteMetadata, not just the options

packages/gatsby-theme-docs/gatsby-config.js Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview April 16, 2020 11:57 Inactive
Copy link
Member

@emmenko emmenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! 💯

@davifantasia davifantasia merged commit 6e980ef into master Apr 16, 2020
@davifantasia davifantasia deleted the rss-feed branch April 16, 2020 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Release Notes: generate RSS feed
3 participants