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 · 3 comments
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!

@ManorSailor
Copy link
Contributor Author

ManorSailor commented Sep 20, 2024

Hey @ArmandPhilippot, thanks again for your input! I really appreciate the thorough response. I mostly agree with your suggestions, but I’d like to revisit a few points.

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.

Type: Page<collection> is a good compromise, I agree. However, what do you think about naming it Page<TCollection> instead? The astro.getActionResult method seems to follow a similar pattern for its return type. That said, while neither option is necessarily more correct, both of them have the following limitations:

  • What if we used data or TData instead of collection? Since paginate works not only with content collections but also with any data fetched from an API, users might confuse collection with Astro's content collections. I could be overthinking this, though—open to your thoughts.
  • Another issue is that we don’t mention that the generic Page type can be anything. If we say something like Page<collection = any>, it could confuse people unfamiliar with TypeScript. On the other hand, if we don’t mention it, we’re deviating from the actual type’s meaning.

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

I’ve spent too much time working with Python’s type annotations and ended up mixing it with TypeScript. Thanks for the correction! Whatever is ultimately decided for Page, I think we can apply the same approach here.

Regarding the remaining points, I don’t have anything else to add.

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.

When you refer to this page, I assume you’re talking about the Routing page? If so, I didn’t notice any other sections on that page containing full API references. For example, the rewrite section links back to its own API reference, but that’s for a standalone method, not an object like Page.

I don’t have anything else to add at this point. Let’s wait for input from the maintainers.

That’s all for now!

Thanks again for your reply. While reading it, I really appreciated how thoughtful your suggestions are. They’re clearly aligned with what the average user would expect.

@ArmandPhilippot
Copy link
Contributor

However, what do you think about naming it Page<TCollection> instead? The astro.getActionResult method seems to follow a similar pattern for its return type.

Why not, I don't have a strong opinion on this. The reason I suggested Page<collection> was because I was influenced by the types used for getCollection(), getEntry, etc.
As long as we don't only use T but a word I think it will help those less familiar with Typescript to understand the type.

And maybe we should update all mentions of CollectionEntry<collection> with CollectionEntry<Collection> to use a consistent font case.

  • What if we used data or TData instead of collection? Since paginate works not only with content collections but also with any data fetched from an API, users might confuse collection with Astro's content collections. I could be overthinking this, though—open to your thoughts.
  • Another issue is that we don’t mention that the generic Page type can be anything. If we say something like Page<collection = any>, it could confuse people unfamiliar with TypeScript. On the other hand, if we don’t mention it, we’re deviating from the actual type’s meaning

Ah indeed, I hadn't thought about data retrieved from an API directly! I was thinking "collections" after rereading the description below (creates one URL for every page of the paginated collection)... I didn't go as far as the example. In this case, Page<Data> or Page<TData> seems more appropriate to avoid confusion. And it matches the property the type applies to!

As for = any I don't know if it's useful with Data/TData which is therefore more vague. A data could be anything. It seems implicit to me, but why not.

Afterwards, maybe I'm overthinking... In Routing, we have Page<T = any> so we could keep the same type. It's just that, if we're going to update it, it might as well be meaningful to the greatest number of people.

Generally, we try to keep the type indicated in the sources. When it involves Typescript generics, we try to simplify or modify slightly to make it easier to read. If it is too complicated, we omit the Type: as you noticed with the paginate function. 😓

When you refer to this page, I assume you’re talking about the Routing page?

Oh no, sorry for the confusion. I was referring to API Reference: if we move the Complete API reference section (or at least just the code block) from Routing to API Reference then it wouldn't make sense to have this block for The pagination page prop section and not do the same with the other properties listed on API Reference (e.g. Astro, context, CollectionEntry, etc.).
(Basically, that's what I was saying in the issue you mentioned. This was already a point that made us hesitate.)

So what I was suggesting was to just delete that block since we have a link to API Reference which details all the properties. I'm not sure the full type is useful in a guide. If anyone wants more details, the reference is there for that I think.

To resume:

1. The pagination page prop section should have a Type:

We agree to add a Type: here but we are hesitant between Page<Data>, Page<TData> or even to specify = any (e.g. Page<TData = any>).

2. The page.data property should have a more explicit Type:

If we mention the generic for page, here too we could specify the nature of page.data (e.g. Array<TData>).

Also, since data() is wrong, we need to replace the description with:

Array of data returned from the `paginate()` function for the current page.

3. A hint should be added to explain how to type the array returned by page.data when using Content Collections.

If my suggestion works with @ManorSailor issue, we could add an aside with a link to Typescript reference something like:

:::tip
When using Typescript, you will need the `GetStaticPaths` type helper to access your data with type safety. You can see how to use it in the [Typescript reference](https://docs.astro.build/en/guides/typescript/#infer-getstaticpaths-types).
:::

I'd say we could put this "tip" in the getStaticPaths section but we already have a caution block at the end... so maybe a better place would be right after the example. However having only two lines of text between the two blocks might seem odd.

4. Missing signature information for the paginate function

I think we agree that the type is too complex to be useful in documentation.

5. Missing argument from the paginate list

If I am not mistaken, a reformulation is sufficient here:

paginate() has the following options:
    - 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

6. Routing: the Complete API reference seems redundant

It seems that we share the same opinion on this point: this section seems redundant with the API reference and the link seems enough. Maybe we should remove it? (see also #8929)

7. (added by myself) Make CollectionEntry type consistent

If we have to edit api-reference, we might as well take advantage of it: maybe we should update all mentions of CollectionEntry<collection> with CollectionEntry<Collection> to use a consistent font case regarding Typescript generics.

Furthermore, when using CollectionEntry, we find two variants:

  • CollectionEntry<collection>
  • CollectionEntry<TCollectionName>

Maybe we should only use one?

So either CollectionEntry<Collection> or CollectionEntry<TCollectionName>...


@ManorSailor Feel free to check that I haven't forgotten anything in the summary and that you agree with what I wrote.

If we agree on these points, I have nothing else to add from my side while waiting for further feedback!

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