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

Improvements to Pagination Documentation: Type Information, Argument Descriptions, and Redundant Sections #9442

Open
ManorSailor opened this issue Sep 18, 2024 · 1 comment
Labels
improve documentation Enhance existing documentation (e.g. add an example, improve description)

Comments

@ManorSailor
Copy link
Contributor

📚 Subject area/topic

Pagination - API Reference

📋 Page(s) affected (or suggested, for new content)

https://docs.astro.build/en/reference/api-reference/#the-pagination-page-prop
https://docs.astro.build/en/reference/api-reference/#paginate
(Potentially) https://docs.astro.build/en/guides/routing/#complete-api-reference

📋 Description of content that is out-of-date or incorrect

A couple of days ago, I was implementing pagination in an Astro-based project on GitHub. While working on it, I faced a few challenges related to the documentation on pagination. I was able to resolve them by exploring the documentation, the Astro codebase, or a combination of both. After addressing the issues, I realized that these changes could be reflected in the documentation to prevent similar challenges for future users. Initially, I planned to create a PR right away, but the scope of this task felt slightly beyond the realm of "issue-less PRs." Therefore, I’ve created this issue first for approval.

On API Reference:

Under #Page Props:

  1. Missing type information for Page prop
    • Just looking at the reference page, it is not immediately clear that Page is a generic type.
    • Suggestion is to include a Type attribute under the title like: Type: Page<T = any>
  2. Similarly, page.data property simply mentions Array. To be more precise, this could be updated to Array[T] following the changes mentioned above.
  3. Incorrect mention of page.data prop as a function in its description. Perhaps, the intent was to mention either paginate or getStaticPaths function?
  4. Lastly, one of the challenges I faced was typing the array returned by page.data when using Content Collections. It may have been my oversight, but it was not immediately obvious that I had to use CollectionEntry utility type. This could be worth mentioning in the reference page... or perhaps in the Pagination section under Routing guide.

Under #paginate:

  1. Missing signature information for the paginate function
    • Looking at PaginateFunction#L2834 type in the codebase, I realize why it was left out as the type is quite complex. I am not so well versed in Astro, so not sure how this could be tackled.
  2. Missing argument from the paginate list:
paginate() has the following arguments:
    - pageSize - The number of items shown per page (10 by default)
    - params - Send additional parameters for creating dynamic routes
    - props - Send additional props to be available on each page

The reference mentions the data argument before the example, but it is missing from the list above. If the goal is to highlight optional arguments, then the wording could be improved.

On Routing guide, under Pagination#Complete API Reference:

This section seems redundant. Issue #8929 addressed the same concern, and the conclusion was to keep it due to the lack of a better solution. However, reading through the comments, it seems the pagination section previously didn't link back to the API reference page—perhaps the main reason for keeping it. Here are a few reasons why I think removing this section entirely might be beneficial:

  • It adds to the maintenance cost, as any modifications need to be reflected across two pages. Failure to do so could again result in inaccurate information.
  • The guide already mentions the most common properties. If the user wants to learn more, there is already a link to the reference page
  • It doesn't seem to fit well in the "guide" section. Perhaps it's just me, but when I want to understand the structure of types, I typically consult the reference pages. This might not be the compelling argument, though.

As @sarah11918 suggested, we could move this type block into the reference page, as it does provide a useful high-level overview of the Page API. However, another argument can be made that it might be better to link to the actual type in the Astro codebase to avoid further maintenance costs.

🖥️ Reproduction in StackBlitz (if reporting incorrect content or code samples)

No response

@ManorSailor ManorSailor added the improve documentation Enhance existing documentation (e.g. add an example, improve description) label Sep 18, 2024
@ArmandPhilippot
Copy link
Contributor

ArmandPhilippot commented Sep 18, 2024

Hi @ManorSailor, thanks for this detailed issue! I'm not an Astro maintainer but I helped to improve the type information on the API reference page. I'd like to clarify some points regarding this page.

1. Missing type information for `Page` prop
   
   * Just looking at the reference page, it is not immediately clear that `Page` is a `generic` type.
   * Suggestion is to include a `Type` attribute under the title like: `Type: Page<T = any>`

Edit: I answered a bit quickly, I didn't remember that the Page type was exported. Knowing that, and like point 2, I would suggest: Page<collection> if you think it might help.

This remark about the first point is a bit outdated (see above: Edit:), but I leave it there if it can help with the reflection >

Page<T = any> does not match the type of the page object. The type would be:

export interface Page<T = any> {
	/** result */
	data: T[];
	/** metadata */
	/** the count of the first item on the page, starting from 0 */
	start: number;
	/** the count of the last item on the page, starting from 0 */
	end: number;
	/** total number of results */
	total: number;
	/** the current page number, starting from 1 */
	currentPage: number;
	/** number of items per page (default: 10) */
	size: number;
	/** number of last page */
	lastPage: number;
	url: {
		/** url of the current page */
		current: string;
		/** url of the previous page (if there is one) */
		prev: string | undefined;
		/** url of the next page (if there is one) */
		next: string | undefined;
		/** url of the first page (if the current page is not the first page) */
		first: string | undefined;
		/** url of the next page (if the current page in not the last page) */
		last: string | undefined;
	};
}

This is a very long type... and writing every properties in Type: could be more noisy than helpful. So we thought that adding the type here was not useful since the different properties of the page object are detailed just below.

That said, perhaps we can improve the description below to help better understand what it is about. Here is a suggestion based on the existing description:

Pagination will pass a `page` prop to every rendered page that represents a single page of data in the paginated collection. This property is an object that includes the data that you’ve paginated (page.data) as well as metadata for the page (page.url, page.start, page.end, page.total, etc). This metadata is useful for things like a “Next Page” button or a “Showing 1-10 of 100” message. Below is a more detailed explanation for each property available.

What do you think? Would it have been clearer for you at the time of implementation?

2. Similarly, `page.data` property simply mentions `Array`. To be more precise, this could be updated to `Array[T]` following the changes mentioned above.

To be more precise, the type would be T[] or Array<T> in Typescript notation.

However, not all users are familiar with Typescript. So we thought that Array would be enough to help the greatest number. The description is there to provide more details. But indeed, the description may not provide enough details... This can be tricky without bringing up Typescript generics.

Here are the two propositions that come to mind as I write.

  • since some other Type: information uses a notation close to what Typescript uses, we could do the same by using something like: Array<collection> (which might be more meaningful than T for those not familiar with Typescript)
  • if the first point is not suitable, we could improve the description but for now I have no suggestions.
3. Incorrect mention of `page.data` prop as a function in its description. Perhaps, the intent was to mention either `paginate` or `getStaticPaths` function?

I agree, there is a problem here! This should be fixed. I think in this case paginate() would be more accurate. Here a temporary suggestion (to be updated following the previous point):

Array of data returned from the `paginate()` function for the current page.
4. Lastly, one of the challenges I faced was typing the array returned by `page.data` when using Content Collections. It may have been my oversight, but it was not immediately obvious that I had to use `CollectionEntry` utility type. This could be worth mentioning in the reference page... or perhaps in the Pagination section under `Routing` guide.

I'm not sure I understand this point. The typing should be automatic.

Ah I think I understand now. You're using Typescript and it infers the type to be never, right?

There is Typescript Reference page that helps with this issue. You don't need CollectionEntry but you should use GetStaticPaths type. Something like:

export const getStaticPaths = (async ({ paginate }) => {
  const posts = await getCollection('blog');

  return paginate(posts, { pageSize: 10 });
}) satisfies GetStaticPaths;

const { page } = Astro.props;

The page object from Astro.props is now correctly typed.

Maybe a "tip" under paginate() or getStaticPaths() linking to Typescript reference could help with this issue...

Under #paginate:

1. Missing signature information for the `paginate` function
   
   * Looking at [PaginateFunction#L2834](https://github.com/withastro/astro/blob/8d4eb95086ae79339764d02215f84f4f21b96edc/packages/astro/src/%40types/astro.ts#L2834) type in the codebase, I realize why it was left out as the type is quite complex. I am not so well versed in Astro, so not sure how this could be tackled.

Yes for such complicated types, we preferred not to put anything... It's not easy to simplify without losing information... and again, you have to think about those who are used to Typescript and those who are not. 😅

2. Missing  argument from the `paginate` list:
paginate() has the following arguments:
    - pageSize - The number of items shown per page (10 by default)
    - params - Send additional parameters for creating dynamic routes
    - props - Send additional props to be available on each page

The reference mentions the data argument before the example, but it is missing from the list above. If the goal is to highlight optional arguments, then the wording could be improved.

You are right, there is a problem with the wording. The properties listed are not arguments but options.

paginate(data, options)

Where, options could be:

{
    pageSize: 10,
    params: { /* some additional params */ },
    props: { /* some additional props */ },
}

On Routing guide, under Pagination#Complete API Reference:

This section seems redundant. Issue #8929 addressed the same concern, and the conclusion was to keep it due to the lack of a better solution. However, reading through the comments, it seems the pagination section previously didn't link back to the API reference page—perhaps the main reason for keeping it. Here are a few reasons why I think removing this section entirely might be beneficial:

* It adds to the maintenance cost, as any modifications need to be reflected across two pages. Failure to do so could again result in inaccurate information.

* The guide already mentions the most common properties. If the user wants to learn more, there is already a link to the reference page

* It doesn't seem to fit well in the "guide" section. Perhaps it's just me, but when I want to understand the structure of types, I typically consult the reference pages. This might not be the compelling argument, though.

As @sarah11918 suggested, we could move this type block into the reference page, as it does provide a useful high-level overview of the Page API. However, another argument can be made that it might be better to link to the actual type in the Astro codebase to avoid further maintenance costs.

On this point I rather agree... but there are drawbacks (that's why I didn't insist in the issue you quote, I understand Sarah's reluctance).

The main problem is that if we move this block to the API reference page, we would have to do the same thing with all the properties listed on this page. It doesn't make sense to do it for one but not the other but it could make reading more complicated for users. So I'm not sure it's the best thing to do...

However, since we detail the properties in the reference I am still not sure that this block is an added value... I tend to agree that the link is enough.

Regarding links to the source code, I am hesitant... The less experienced might be scared to find themselves in the source code while an idea of ​​the expected types might be useful to them. Providing the information directly in the documentation seems a good compromise. Even if it adds maintenance...

Again, thanks for this detailed issue! 👍🏽 I agree that there are some improvements/fixes to be made. Feel free to react to my remarks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improve documentation Enhance existing documentation (e.g. add an example, improve description)
Projects
None yet
Development

No branches or pull requests

2 participants