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: Add parseAsPageIndex parser #791

Merged
merged 14 commits into from
Feb 9, 2025
Merged

feat: Add parseAsPageIndex parser #791

merged 14 commits into from
Feb 9, 2025

Conversation

cenobitedk
Copy link
Contributor

Hi! Thanks for a great package!

I have suggestion for a (probably very) common use-case where you add the current page (eg. of a table) to the url. At least if you are doing pagination client-side. Typically you have the page as 0-index yet you want the url to be human readable starting from 1. Here parseAsPageIndex comes in super handy, taking care of the +1 offset automatically.

For example, let's say you use @tanstack/react-table and want to have the pagination as part of the url:

  const [pagination, setPagination] = useQueryStates(
    {
      pageIndex: parseAsPageIndex.withDefault(0),
      pageSize: parseAsInteger.withDefault(15),
    },
    {
      urlKeys: {
        pageIndex: 'page',
        pageSize: 'size',
      },
    },
  );

I was also thinking about parseAsIntegerWithOffset with any offset number, but I could only think of very few situations where it would be beneficial.

Copy link

vercel bot commented Dec 4, 2024

@cenobitedk is attempting to deploy a commit to the 47ng Team on Vercel.

A member of the Team first needs to authorize it.

@cenobitedk
Copy link
Contributor Author

The only challenge I've found is that setPagination returns a Promise which @tanstack/react-table doesn't like.
So it must be wrapped like this:

    onPaginationChange: (p) => {
      setPagination(p);
    },

Here's the full example:

  const [pagination, setPagination] = useQueryStates(
    {
      pageIndex: parseAsPageIndex.withDefault(0),
      pageSize: parseAsInteger.withDefault(15),
    },
    {
      urlKeys: {
        pageIndex: 'page',
        pageSize: 'size',
      },
    },
  );

  const table = useReactTable({
    columns,
    data,
    getCoreRowModel: getCoreRowModel(),
    getPaginationRowModel: getPaginationRowModel(),
    onPaginationChange: (p) => {
      setPagination(p);
    },
    state: {
      //...
      pagination,
    },
  });

@franky47 franky47 changed the title Add parseAsPageIndex parser feat: Add parseAsPageIndex parser Feb 1, 2025
Copy link
Member

@franky47 franky47 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

nitpick: What do you think about naming it parseAsIndex? The zero-based approach might work for other things than pagination, though I agree it's likely to be the main use-case. parseAsIndex might be less clear of what it does though (but that's what documentation is for).

Copy link

vercel bot commented Feb 7, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nuqs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 9, 2025 0:19am

@cenobitedk
Copy link
Contributor Author

What do you think about naming it parseAsIndex?

Fine with me! :)

@cenobitedk
Copy link
Contributor Author

@franky47 The contribution doc is a little vague on what to add before a feature is ready. I have added the parser and test, and I have the documentation ready as well. But what about e2e test, should I update those as well?

@cenobitedk
Copy link
Contributor Author

@franky47 Hi François. I've updated e2e tests for Next.js. As far as I can tell, everything should be ready to go 👍
If you can trigger the checks, that'd be awesome!

@franky47
Copy link
Member

franky47 commented Feb 8, 2025

Thank you! The e2e part wasn't really necessary as parsers can easily be unit-tested, I need to clean the e2e-next project and remove duplicates. The e2e envs are mostly to test the adapters and framework behaviours, I'll update the contribution docs to reflect that.

I see the Prettier linter step is failing on the parsers file, would you mind taking a look? Thanks!

@cenobitedk
Copy link
Contributor Author

I see the Prettier linter step is failing on the parsers file, would you mind taking a look? Thanks!

Sure! I tried to address it in the last commit. However the output from the check was not clear as to what was wrong and when I run pnpm test I get no problems:

image image

But I suspect it has to do with semicolon and other small things.

@franky47 franky47 added this to the 🪵 Backlog milestone Feb 8, 2025
@franky47 franky47 added feature New feature or request parsers/built-in Related to built-in parsers labels Feb 8, 2025
@franky47
Copy link
Member

franky47 commented Feb 9, 2025

Ok I might need to improve that part a bit more too then, so all tests in CI can be run locally.

If you had Prettier format the files locally and it doesn't pass CI linting, maybe we have a config issue where user-set global Prettier config takes precedence and conflicts with defaults.

Copy link
Member

@franky47 franky47 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! I added a negative index check to the parser, to avoid out-of-bounds access.

const [value, setValue] = useQueryState('page', parseAsIndex)
return (
<DemoContainer demoKey="page">
<input
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: Add a minimum of zero for the input, as the parser now checks for positive-only values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A minimum of zero makes sense for page index, but as a general index, a value -1 or below could be utilised to iterate backwards on an array. Something like .withOptions({ min: 0}) would make sense. Was that what you had in mind?

Copy link
Member

@franky47 franky47 Feb 9, 2025

Choose a reason for hiding this comment

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

Yeah I thought about going negative to do python-like backwards indexing (using .at), but it would mean:

  • ?page=0 maps to the last element (index -1)
  • ?page=-1 maps to the second to last element (index -2)

This seems even more confusing.

I'll remove the positive check and we can enforce runtime values with validation once #446 (or a variant) lands.

Note: TanStack Table doesn't seem to do a bounds check on negative indices: https://table.sadmn.com/?flags=advancedTable&page=-1

@franky47 franky47 enabled auto-merge (squash) February 9, 2025 12:16
@franky47 franky47 merged commit 11a46bf into 47ng:next Feb 9, 2025
23 checks passed
@cenobitedk
Copy link
Contributor Author

Awesome! Thanks for merging 🙏🏻 happy to contribute

Copy link

🎉 This PR is included in version 2.4.0-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Copy link

🎉 This PR is included in version 2.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@franky47 franky47 removed this from the 🚀 Shipping next milestone Feb 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request parsers/built-in Related to built-in parsers released on @beta released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants