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

[Content Collections] Add slug frontmatter field #5941

Merged
merged 14 commits into from
Jan 23, 2023

Conversation

bholmesdev
Copy link
Contributor

Changes

  • Remove slug() collection configuration option
  • Add special handling for the slug frontmatter property
    • Override generated slug with the slug frontmatter property when present
    • Respect slug during type generation
    • Exclude slug from schema parsing
  • Raise error when slug is present in a schema. Otherwise, users may attempt to use .transform() to manipulate slugs, and be surprised when this is not respected
  • Chore: add new 9xxx error heading for content collection errors

Testing

Update tests to use slug frontmatter property

Docs

  • New error messages
  • TODO: docs updates

@bholmesdev bholmesdev requested a review from a team as a code owner January 23, 2023 15:46
@changeset-bot
Copy link

changeset-bot bot commented Jan 23, 2023

🦋 Changeset detected

Latest commit: 43b6159

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 Jan 23, 2023
Copy link
Contributor

@matthewp matthewp left a comment

Choose a reason for hiding this comment

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

Blocking for now so we can talk about with core.

@matthewp
Copy link
Contributor

Given the timing of this, I think adding a new feature to make slug use-cases better is not the right move. We don't have time to beta test this, or to really even discuss it and understand it ourselves.

I'd feel more comfortable with doing one of these two things:

  1. Make no changes to data/slug. getEntryBySlug is O(n) right now. We might be able to fix this in the future. If not we can create a new API that is designed to be O(1) and deprecate getEntryBySlug.

  2. Get rid of the data/slug as previously discussed. There isn't a great way to do custom slugs that derive from frontmatter, but we can follow-up in a 2.x release to improve this.

@matthewp
Copy link
Contributor

I'm increasing feeling like (1) is the way to go. Before the change from last week we were going to tell people to use getCollection and filter themselves, so the O(n) problem already existed. Now we've inherited the problem ourselves and have a chance at fixing it. Worst case we can't and design a new API that can.

@bholmesdev
Copy link
Contributor Author

I understand your points! After our discord discussion, it sounds like making custom slug mapping performant will be possible with today's implementation. However, I want to highlight a few reasons this isn't ideal:

  • Introducing a reserved property like slug will be a breaking change. This is our only chance until 3.0 to consider such an option.
  • Discussing with the docs team, the slug() configuration option will mostly be used to map a slug frontmatter property to the generated slug anyways. This is supported by the content of Jason and Jared's tutorials. Given this, a new design would be informed by our beta findings.
  • The slug() configuration option removes type generation, making slug a generic string. This change adds smart type generation back.

@FredKSchott
Copy link
Member

FredKSchott commented Jan 23, 2023

I can share some context/experience from having worked on the Content Collections API docs these last few days. All attempts to document this specific feature have been pretty tough, and I've been struggling to figure out what to do about:

  1. We need to document how you customize the slug in two places, based on whether you need frontmatter or not.
  2. We add a note that customizing slugs today is not feasible in larger collections in SSR, since in requires calling getCollection() on every request.

I agree this is late in the game, but this is actually the API that I was leaning towards personally before I even saw this PR come in, so I'm a bit biased to it. It addresses the most post popular use-case for the feature in a way that is more easily made performant behind-the-scenes in future versions. I think the decision to ship an API that we're not happy with deserves special consideration, even this late in the game.

It's not the end of the world if this doesn't get merged for 2.0: we could mark slug as a reserved frontmatter property (not allowed to use it) now so that we can implement this in 2.1 as a non-breaking change. BUT... this is an API that I'm more confident in, and I think it's worth the risk (or at least the discussion) that we'd be taking on to try to make it happen.

@matthewp
Copy link
Contributor

Putting aside whether this API is better or not (I think it probably is), just want to address the backwards compatible comment. We can add new reserved properties by changing how they are used within a schema, for example if we added a type property to defineCollection: defineCollection({ type: 'page', schema }) or created a definePage({ schema }), etc., this would be a new feature and frontmatter could have special rules in this collection.

So we can keep things backwards compatible by adding new APIs for special use-cases rather than just modifying the existing APIs (which would not be compatible).

@matthewp
Copy link
Contributor

I think I feel more confident given that 2 people have seen similar problems with the existing API. Given that this is about API as much as it is about perf that's a stronger reason to do the unconventional thing and make a last minute change.

My previous hesitancy was based on:

  1. Timing; still hesitant here but with 2 people agreeing on this change it makes me feel better. It doesn't remove functionality but just moves it to another place.
  2. Concern about further entrenching the idea of slug into the API where it felt like collections should stay out of use-cases. Given that slug is already part of the API through the schema, this doesn't make slug further entrenched, just moves its location.

So given that, I'm more neutral/positive on the change.

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.

Looks good! One bit of wording seemed too gentle, so make sure the advice to the reader is appropriate for the situation, then it's good to go!

.changeset/large-steaks-film.md Outdated Show resolved Hide resolved
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
@bholmesdev bholmesdev merged commit 3048238 into main Jan 23, 2023
@bholmesdev bholmesdev deleted the feat/change-custom-slug-handling branch January 23, 2023 20:04
@andersk
Copy link
Contributor

andersk commented Jan 24, 2023

Does this mean I can no longer generate slugs programmatically? I had been prepending the date to each slug:

const posts = defineCollection({
  slug: ({ defaultSlug, data: { pubDate } }) =>
    `${formatInTimeZone(pubDate, "UTC", "yyyy/MM/dd")}/${defaultSlug}`,});

Now it seems I have to manually edit this into every individual post.

I guess I can work around this by using an external function getPostLink(post) everywhere instead of post.slug.

@markjaquith
Copy link
Contributor

I guess I can work around this by using an external function getPostLink(post) everywhere instead of post.slug.

That's what I'm planning to do. I have the opposite problem as you... I have dates prepending my markdown files, like 2023-01-26-some-blog-post.md that I use for providing date sorting when viewing the directory, and that I was stripping off the front. Now, I'm removing that when I generate the slug portion of my params in my getStaticPaths() return value, and I'll pass slugs through the same function when linking to them.

@bholmesdev
Copy link
Contributor Author

@andersk @markjaquith Yes, sorry to say manual slugs and-or helpers like getPostLink are the recommended path. This was after a thorough, week-long discussion on finding the best compromise between performance and end-user utility. I recognize this need, and definitely want to work with you all on a sustainable solution going forward. We'll save your input here for future reference, and if you you have solutions to propose, definitely open an RFC discussion!

matthewp pushed a commit that referenced this pull request Jan 26, 2023
* feat: respect `slug` frontmatter prop

* chore: replace `slug` check with proper types

* fix: regen types on `slug` change

* chore: add TODO on slug gen

* tests: update to use `slug` frontmatter prop

* chore: add error message on `slug` inside object schema

* lint

* chore: add note on frontmatter parse

* refactor: move content errors to new heading

* chore: ContentSchemaContainsSlugError

* chore: changeset

* docs: be 10% less gentle

Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>

* fix: avoid parsing slug on unlink

* docs: clarify old API is for beta users

Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
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.

6 participants