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(blog): allow sorting blog posts through a options.sortPosts function hook #9840

Closed
wants to merge 16 commits into from

Conversation

OzakIOne
Copy link
Collaborator

@OzakIOne OzakIOne commented Feb 9, 2024

Pre-flight checklist

  • I have read the Contributing Guidelines on pull requests.
  • If this is a code change: I have written unit tests and/or added dogfooding pages to fully verify the new behavior.
  • If this is a new API or substantial change: the PR has an accompanying issue (closes #0000) and the maintainers have approved on my working plan.

Motivation

Allow the user to pass a function to sort blog posts, until now, users only had the choice between ascending and descending sorting options, we want to provide to the user the possibility to pass a function which enable other sorting order

Todo:

  • check for invalid preset ? check if typescript already protects for this, otherwise create a getPreset function
  • move presets outside of function
  • put a type for the map input enum output sort function
  • put blogpost as params for preset, presets should have the same signature of the sort function,
    record ascending descending to blogpost sort preset, call sort function like preset function
  • dogfooding test

Test Plan

Test links

Deploy preview: https://deploy-preview-9840--docusaurus-2.netlify.app/

Related issues/PRs

#9831

#5787

#5692

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Feb 9, 2024
@OzakIOne OzakIOne changed the title wip feat: sortPosts function config Feb 9, 2024
Copy link

github-actions bot commented Feb 9, 2024

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🟠 69 🟢 98 🟢 96 🟢 100 🟠 88 Report
/docs/installation 🟠 89 🟢 96 🟢 100 🟢 100 🟠 88 Report
/docs/category/getting-started 🟠 75 🟢 100 🟢 100 🟢 90 🟠 88 Report
/blog 🟠 67 🟢 100 🟢 100 🟢 90 🟠 88 Report
/blog/preparing-your-site-for-docusaurus-v3 🟠 60 🟢 96 🟢 100 🟢 100 🟠 88 Report
/blog/tags/release 🟠 71 🟢 100 🟢 100 🟠 80 🟠 88 Report
/blog/tags 🟠 76 🟢 100 🟢 100 🟢 90 🟠 88 Report

Copy link

netlify bot commented Feb 9, 2024

[V2]

Built without sensitive environment variables

Name Link
🔨 Latest commit c949c3f
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/65d8b81913407600085c4278
😎 Deploy Preview https://deploy-preview-9840--docusaurus-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

packages/docusaurus-plugin-content-blog/src/blogUtils.ts Outdated Show resolved Hide resolved
Comment on lines 447 to 448
sortPosts: (a: BlogPost, b: BlogPost): number =>
b.metadata.date.getTime() - a.metadata.date.getTime(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Test this by applying this to the dogfood blog plugin.

For example you can write a blog post with an old date, and make it appear in 1st position thanks to a front matter field?

packages/docusaurus-plugin-content-blog/src/blogUtils.ts Outdated Show resolved Hide resolved
@OzakIOne OzakIOne changed the title feat: sortPosts function config feat: implement function support for sorting blogpost Feb 14, 2024
packages/docusaurus-plugin-content-blog/src/blogUtils.ts Outdated Show resolved Hide resolved
packages/docusaurus-plugin-content-blog/src/blogUtils.ts Outdated Show resolved Hide resolved
packages/docusaurus-plugin-content-blog/src/blogUtils.ts Outdated Show resolved Hide resolved
packages/docusaurus-plugin-content-blog/src/blogUtils.ts Outdated Show resolved Hide resolved
return customSort;
}
} else if (sortPresets[sortPosts]) {
return sortPresets[sortPosts](blogPosts);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Your presets sort blog posts imperatively and do not return a list of blog posts. IE when using presets the returned value is undefined.

I'd prefer if this function consistently returned a list of sorted blog post. If the input list is mutated without creating a copy, you'd rather return it.

),
};

function sortBlogPosts({
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 good unit-test coverage of this function, covering both mutation modes, with all possible combinations

  • usage of a preset
  • usage of imperative mutation sort function (sort the input list, return undefined)
  • usage of fp mutation sort function (return a sorted blog post list copy)
  • undefined sort function

What about another helper function that resolves preset names to their respective sort functions?

function getSortFunction(sortPosts: Options["sortPosts"]): SortBlogPostsFn {
  if (typeof sortPosts === 'function') {
    return sortPosts
  }
  if (typeof sortPosts === "string") {
   const presetFn = presets[sortPosts];
   if (!presetFn) {
     throw new Error("sortPosts preset "+sortPosts+" does not exist");
   }
   return presetFn;
  }
  // a sort function that does nothing and keeps the initial order
  return () => {};
}

website/docs/api/plugins/plugin-content-blog.mdx Outdated Show resolved Hide resolved
packages/docusaurus-plugin-content-blog/src/blogUtils.ts Outdated Show resolved Hide resolved
#### `SortBlogPostsFn` {#SortBlogPostsFn}

```ts
// TODO document this type
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// TODO document this type

We don't document the BlogPost type yet, cf CreateFeedItemsFn above.

We probably should but I'd consider it's not the responsibility of this PR to add this doc, because we need to think a bit about the public API surface we want to expose/document here.

We'll probably document this later but maybe not all the fields, because we don't necessarily want to lose the ability to rename/remove attributes (cf the formattedDate that we'd like to remove).

For now this is kind of undocumented on purpose, so that we consider it's not public API and we can still refactor in v3.x without considering it as a breaking change.

Copy link
Collaborator

Choose a reason for hiding this comment

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

remove TODO for now


```ts
// TODO document this type
((args: {blogPosts: BlogPost[]}) => void | BlogPost[])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
((args: {blogPosts: BlogPost[]}) => void | BlogPost[])
type SortBlogPostsFn = (params: {blogPosts: BlogPost[]}) => void | BlogPost[]

Follow the declaration convention of type params above: give a name to type and use params instead of args (also in code if possible)

Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

Maybe extending sortPosts is not the API we want, and instead, it would be better to directly implement a processBlogPosts() hook (or any other name?).

Technically it would do the same, but the name would be broader, not limited to sorting.

What do you think @OzakIOne @Josh-Cena @ilg-ul ?

The way I see it, the existing sortPosts preset option would be applied earlier because it's less powerful.

let blogPosts = await loadBlogPosts();
blogPosts = applySortPreset(blogPosts,options.sortPosts); 
blogPosts = applyProcessing(blogPosts,options.processBlogPosts); 
// ...

Eventually we could later deprecated options.sortPosts considering you can already sort posts with options.processBlogPosts

Comment on lines 281 to 304
{
id: 'newest',
metadata: {
permalink: '/blog/2018/12/14/Happy-First-Birthday-Slash',
source: path.posix.join(
'@site',
pluginDir,
'2018-12-14-Happy-First-Birthday-Slash.md',
),
title: 'Happy 1st Birthday Slash!',
description: `pattern name`,
date: new Date('2018-12-14'),
tags: [],
prevItem: {
permalink: '/blog/2019/01/01/date-matter',
title: 'date-matter',
},
hasTruncateMarker: false,
frontMatter: {},
authors: [],
unlisted: false,
formattedDate: '',
},
content: '',
Copy link
Collaborator

Choose a reason for hiding this comment

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

too verbose test code, create a helper that can accept partial blog post args and let us focus on what matters

Your test should be as readable as this (pseudo code), with the intent being super clear

it("sorts descending by default",() => {
  const blogPost2022 = createBlogPost({date: new Date('2022-12-14')});
	const blogPost2023 = createBlogPost({date: new Date('2023-12-14')});
	const blogPost2024 = createBlogPost({date: new Date('2024-12-14')});

  assert(sort([
	  blogPost2022,
	  blogPost2024,
	  blogPost2023,
  ])).equals([
		blogPost2024,
		blogPost2023,
		blogPost2022
	])
});

It's preferable if each test create its own inputs, instead of relying on a shared list of posts for all tests. As we add tests, this list will only grow over time and tests will become harder to understand and maintain

#### `SortBlogPostsFn` {#SortBlogPostsFn}

```ts
// TODO document this type
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove TODO for now

const sortedBlogPosts = sortFunction({blogPosts});

if (sortedBlogPosts !== undefined) {
if (sortedBlogPosts.length === 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure why someone would return an empty array by mistakes.

Users are likely to either return undefined, or return a new sorted list of posts.

If we want to prevent mistakes, we should check that blogPosts.length === sortedBlogPosts.length.

We could check that the returned value is an array, because users might use JS in config files and return an unsafe value such as a number 🤷‍♂️


But I'm not sure we want to prevent that anyway, because sortPosts could also be used as a temporary workaround to filter/modify posts until we come up with a better API for it.
It's not the primary use case of this API, and it's a bit awkward to do so.
See my comment here:
#9846 (comment)

Maybe sortPosts is not the API we should extend?

@slorber slorber changed the title feat: implement function support for sorting blogpost feat(blog): allow sorting blog posts through a options.sortPosts function hook Feb 20, 2024
@slorber slorber added pr: new feature This PR adds a new API or behavior. to backport This PR is planned to be backported to a stable version of Docusaurus labels Feb 20, 2024
@ilg-ul
Copy link
Contributor

ilg-ul commented Feb 20, 2024

it would be better to directly implement a processBlogPosts() hook (or any other name?)

Or even use the more generic afterContentLoaded(), as suggested in #9863?

@slorber
Copy link
Collaborator

slorber commented Feb 20, 2024

it would be better to directly implement a processBlogPosts() hook (or any other name?)

Or even use the more generic afterContentLoaded(), as suggested in #9863?

I've thought about it, and it's a potentially good flexible low-level solution, but we are not ready for it and it's much more complicated to implement.

Notably, the blog post content historically has this shape:

  export type BlogContent = {
    blogSidebarTitle: string;
    blogPosts: BlogPost[];
    blogListPaginated: BlogPaginated[];
    blogTags: BlogTags;
    blogTagsListPath: string;
  };

You will notice that the loaded content is "already paginated". If you apply content.blogPosts.sort(...) after content loading, the order you define will not impact the paginated display order, which is unexpected.

So, for now, I'd prefer to focus on a convenient high-level feature to sort/filter/modify blog posts through plugin options, rather than a generic plugin solution to do the same on plugin-loaded content. I'd still like to make this generic solution possible in the long term but it's challenging and I'm not sure how yet.

@slorber
Copy link
Collaborator

slorber commented Feb 26, 2024

Closing in favor of #9886

@slorber slorber closed this Feb 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: new feature This PR adds a new API or behavior. to backport This PR is planned to be backported to a stable version of Docusaurus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants