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

Deprecate drafts feature #8099

Merged
merged 7 commits into from
Aug 22, 2023
Merged

Deprecate drafts feature #8099

merged 7 commits into from
Aug 22, 2023

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Aug 16, 2023

Changes

Deprecate drafts feature in favour of content collections

Testing

Tested manually that the warning is logged in dev and build.

Docs

Added a changeset. The migration path is a bit bigger since it involves migrating to content collections first, then filtering draft. I linked to the related parts in docs instead.

I'm also not sure if we need more documentation in the added @deprecated jsdoc. If we have more info in the published migration guide later, those fields could point to it instead?

/cc @withastro/maintainers-docs for feedback!

@bluwy bluwy requested a review from a team as a code owner August 16, 2023 09:35
@changeset-bot
Copy link

changeset-bot bot commented Aug 16, 2023

🦋 Changeset detected

Latest commit: 4435deb

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

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Aug 16, 2023
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.

Hey @bluwy! Quick touch up to the changeset, and I've left a question for @delucis since I can't remember the last time we deprecated something on this page and I want to make sure we're doing it correctly. 😄

.changeset/silent-bikes-crash.md Outdated Show resolved Hide resolved
@@ -1017,6 +1017,7 @@ export interface AstroUserConfig {
* @name markdown.drafts
* @type {boolean}
* @default `false`
* @deprecated since version 3.0. Use content collections instead.
Copy link
Member

Choose a reason for hiding this comment

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

@delucis - Is there any special formatting we need to adhere to for the deprecated entry here?

Copy link
Member

Choose a reason for hiding this comment

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

We don’t actually display @deprecated comments in docs yet, so now’s our chance to decide a format if we want!

I can imagine generating something like this? Where the contents of the comments follow the leading Deprecated:.

image

Or even something like this depending on how highlighted we want it:

image

So given that, I guess it depends if you think specific wording would be clearest there @sarah11918?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, checking where I adopted the since version X pattern which is from errors-data.ts, it looks like we already render it today:

image

So perhaps another place to align if we want to make @deprecated work nicely everywhere.

Copy link
Member

Choose a reason for hiding this comment

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

Ah OK, yes, we could implement a similar pattern in the config reference if we like how the error generation script does this.

In this example I don’t love how basically a full sentence is split across heading and paragraph, so if we like the chunkier aside rendering, I would probably argue we should end up with something that looks more like:

⚠️ Deprecated
Deprecated since version 2.6. Use content collections instead.

In which case, best for that to be reflected in the JSDoc comment too:

Suggested change
* @deprecated since version 3.0. Use content collections instead.
* @deprecated Deprecated since version 2.6. Use content collections instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

I pushed a commit to update the @deprecated tag to be a proper sentence instead 👍

@bluwy
Copy link
Member Author

bluwy commented Aug 17, 2023

Thanks for the review! I've also updated the RSS readme (missed that). Marked the drafts option deprecated too, but I'm not sure if that's consistent with how we usually mark things deprecated.

@sarah11918
Copy link
Member

Just noting that for v3.0, we will actually be using this here: #8158

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 for updating the @deprecated tags @bluwy! LGTM!

@bluwy bluwy merged commit 732111c into next Aug 22, 2023
@bluwy bluwy deleted the deprecate-draft branch August 22, 2023 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants