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

fix: filter out draft item from glob result #5612

Merged
merged 7 commits into from
Dec 27, 2022
Merged

fix: filter out draft item from glob result #5612

merged 7 commits into from
Dec 27, 2022

Conversation

equt
Copy link
Contributor

@equt equt commented Dec 16, 2022

Changes

Testing

A corresponding test has been added to ensure that any Markdown/MDX has draft: true in its frontmatter will be ignored, and a missing one or false will not.

Docs

I'm not sure if this is a new feature or just a fix. Maybe we should add a note in the RSS doc?

/cc @withastro/maintainers-docs

@changeset-bot
Copy link

changeset-bot bot commented Dec 16, 2022

🦋 Changeset detected

Latest commit: a138bdc

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

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

Thanks @equt.

Could we make this configurable via an option to the rss() function? Or perhaps follow the top-level markdown.drafts option although not sure we can easily access that in this context.

I know some people like to do stuff like include drafts when running astro dev to test stuff out, but exclude them in astro build, so making this configurable would support that use case.

.changeset/modern-dodos-invent.md Outdated Show resolved Hide resolved
@equt
Copy link
Contributor Author

equt commented Dec 17, 2022

Agree with that, but I can't figure out a way to get that option in the top-level config unless we parse the astro.config file again in the RSS package.

🤔 How about we take a step back instead? Rather than filtering for the user, we expose the draft in the RSSFeedItem. This would allow something like a preview mode for previewing draft posts, and release mode for excluding all drafts.

import rss from '@astrojs/rss';

const postImportResult = import.meta.glob('../posts/**/*.md', { eager: true });
const posts = Object.values(postImportResult);

export const get = () => rss({
  title: 'Buzz’s Blog',
  description: 'A humble Astronaut’s guide to the stars',
  site: import.meta.env.SITE,
  items: posts.map((post) => ({
    link: post.url,
    title: post.frontmatter.title,
    pubDate: post.frontmatter.pubDate,
  })).filter((post) => {
    switch (import.meta.env.MODE) {
      case 'production':
        return !post.draft
      default:
        return true
    }
  })
});

@fflaten
Copy link
Contributor

fflaten commented Dec 18, 2022

Agree with that, but I can't figure out a way to get that option in the top-level config unless we parse the astro.config file again in the RSS package.

🤔 How about we take a step back instead? Rather than filtering for the user, we expose the draft in the RSSFeedItem. This would allow something like a preview mode for previewing draft posts, and release mode for excluding all drafts.

There's really no need to expose anything in RSSFeedItem, is it? You can already get draft directly from frontmatter. Ex.

const postImportResult = import.meta.glob('./posts/**/*.{md,mdx}', { eager: true });
const posts = Object.values(postImportResult).filter((post) => import.meta.env.DEV || !post.frontmatter.draft);

If anything, I'd personally prefer RSSOptions to have a includeDrafts-bool like @delucis suggested. However, having content collections in mind, filtering gets even easier (see below), is accessed using data vs frontmatter and users might rename draft to isDraft etc. So maybe not the best time to hardcode a filter at all, but rather include steps in docs? 🙂

const posts = (await getCollection('blog', ({ data }) => {
    return import.meta.env.DEV || !data.draft
  }));

@equt
Copy link
Contributor Author

equt commented Dec 19, 2022

There's really no need to expose anything in RSSFeedItem

Indeed, we don't need to expose the draft again, and somehow I just missed that.

users might rename draft to isDraft etc

Considering the markdown integration has already hardcoded this, I guess most users will just go with it.

So maybe not the best time to hardcode a filter at all, but rather include steps in docs?

We have to do something by default. Users who have put the draft: true in their posts will always expect the draft content to be filtered out.

@delucis
Copy link
Member

delucis commented Dec 19, 2022

Fair enough if we can’t get the top-level Markdown config option.

In that case I’d still suggest adding an option to the getRSS() function, so you could do:

import rss from '@astrojs/rss';

export const get = () => rss(
  drafts: true, // Include drafts
  // other options ...
});

@equt equt requested a review from delucis December 20, 2022 02:13
Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

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

Nice! LGTM — thanks @equt 🙌

Would love a second opinion from the core team on whether this is in fact a major bump (the safe choice) or if we can get away with calling this a bug fix.

@@ -28,6 +28,8 @@ type RSSOptions = {
stylesheet?: string | boolean;
/** Specify custom data in opening of file */
customData?: string;
/** Whether to include drafts or not */
drafts?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I'd prefer the name includeDrafts for naming a boolean option

Copy link
Member

Choose a reason for hiding this comment

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

I like this but it would then be inconsistent with our own markdown config which uses drafts: true to build draft files as pages.

I guess we could also rename the markdown flag in the future although markdown.includeDrafts is a less obvious home run in terms of naming.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough! I personally didn't make the connection between the markdown config and the RSS config, so the different naming didn't bother me. I'm okay keeping it as drafts if it makes documentation easier.

Copy link
Contributor

@bholmesdev bholmesdev left a comment

Choose a reason for hiding this comment

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

Left a nit on naming, but I'm happy with this PR's final draft!

To be a good semver samaritan, I'd stick with a major for this one. We could switch the default of includeDrafts to true if we want to merge as a patch. Still, we'd probably flip back to false soon after for a major release, so that doesn't seem appealing to me.

@bholmesdev
Copy link
Contributor

So maybe not the best time to hardcode a filter at all, but rather include steps in docs? 🙂

@fflaten Great point on Content Collections! I think the RSS integration will see some changes as content collections are more widely adopted. A loose set of ideas:

  • Remove the import.meta.glob auto-resolver
  • Introduce an rssSchema people can apply to their existing content config. This allows rich error messages on which properties are required by RSS, instead of the opaque "random entry broke RSS" error you see today:
import { rssSchema } from '@astrojs/rss';

const blog = defineCollection({
  schema: {
    ...rssSchema,
    /* other properties you may want */
  }
});

Excited to work on this more with the community!

@tony-sull
Copy link
Contributor

This looks great! One quick note on the semver question - this should either be a major (if drafts are filtered out by default) or a minor (if drafts are included by default)

The second option is backwards compatible, but since it's really a new feature instead of a bug fix it's better to avoid using a patch release for it 👍

@delucis delucis requested a review from a team as a code owner December 21, 2022 18:02
Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Approving the README on behalf of Astro Docs Maintainers! 🥳

@matthewp matthewp merged commit 68c20be into withastro:main Dec 27, 2022
@astrobot-houston astrobot-houston mentioned this pull request Dec 27, 2022
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.

@astrojs/rss includes drafts in output
7 participants