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

Route link of the first page missing page param. #517

Closed
audiBookning opened this issue May 22, 2021 · 7 comments
Closed

Route link of the first page missing page param. #517

audiBookning opened this issue May 22, 2021 · 7 comments

Comments

@audiBookning
Copy link
Contributor

audiBookning commented May 22, 2021

I just tried the package. It looks very good.

But unfortunately, i just tested it and in the routes links, the "first" is not correct.
"first": "/client/paginated?limit=3",
The page parameter is missing.

Here is the received result a little more expanded.

    "meta": {
        "totalItems": 8,
        "itemCount": 3,
        "itemsPerPage": 3,
        "totalPages": 3,
        "currentPage": 2
    },
    "links": {
        "first": "/client/paginated?limit=3",
        "previous": "/client/paginated?page=1&limit=3",
        "next": "/client/paginated?page=3&limit=3",
        "last": "/client/paginated?page=3&limit=3"
    }

I basically copy pasted the example of the readme file in a new nestjs project to test the package.

Here is the code that i used to try the package: sample-nestjs-typeorm-paginate

@bashleigh
Copy link
Collaborator

oh yea that's not right. What version are you using?

@bashleigh
Copy link
Collaborator

Actually no that's fine. With no page param the default page is 1

@audiBookning
Copy link
Contributor Author

audiBookning commented May 22, 2021

Thanks for taking the time to review the issue.

I received this error when querying:

{
    "statusCode": 400,
    "message": "Validation failed (numeric string is expected)",
    "error": "Bad Request"
}

The following is not an excuse, just an explanation.
It was a little late, the brain was in need of a break and i was doing other things at the same time. I was in an hurry to finish the example before "ending the day".
So i basically just copy pasted the example code and when the error appeared, my brain freezed and i posted this issue without being able to think about it. Sorry.

Now that i review the error, it is easy to see the problem.
The code that i used in the controller was copied from the example given in the readme. I just made a simple adaptation.

@Get('paginated')
  getPaginatedClients(
    @Query('page', ParseIntPipe) page: number = 1,
    @Query('limit', ParseIntPipe) limit: number = 10,
  ) {
    limit = limit > 100 ? 100 : limit;
    return this.clientSvc.getPaginatedClients({
      page,
      limit,
      route: '/client/paginated',
    });
  }

I am using the ParseIntPipe, which is not happy if the page param comes null.
I just have to use another solution for the validation.

As not completely wasting your time, this issue can be converted to a suggestion:

  • Add the page param to the route link.
  • Or, simpler and more logical in terms of soft dev, to simply edit the readme to avoid this situation. The simplest edit would be to remove the ParseIntPipe in at least the page query param. But it would be better to remove it in the limit param also, and in this way avoiding to confuse the "happy copy-pasters" as myself 🙂

Thanks.

@audiBookning
Copy link
Contributor Author

A much better suggestion would be to use the nestjs DefaultValuePipe.
So the readme controller example would be changed to:

import { Controller, DefaultValuePipe, Get, ParseIntPipe, Query } from '@nestjs/common';
import { CatService } from './cat.service';
import { CatEntity } from './cat.entity';
import { Pagination } from 'nestjs-typeorm-paginate';

@Controller('cats')
export class CatsController {
  constructor(private readonly catService: CatService) {}
  @Get('')
  async index(
    @Query('page', new DefaultValuePipe(1), ParseIntPipe) page: number = 1,
    @Query('limit', new DefaultValuePipe(10), ParseIntPipe) limit: number = 10,
  ): Promise<Pagination<CatEntity>> {
    limit = limit > 100 ? 100 : limit;
    return this.catService.paginate({
      page,
      limit,
      route: 'http://cats.com/cats',
    });
  }
}

@bashleigh
Copy link
Collaborator

Oh ok I see now, would you like to create a PR to add this to the docs?

@audiBookning
Copy link
Contributor Author

Sure. It is just a little correction to the readme. I will do it as soon as i can.

@audiBookning
Copy link
Contributor Author

The PR was open, so i will close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants