Conversation
| useUrlStateWithFilter, | ||
| useUrlStateWithSort, | ||
| } from "./"; | ||
|
|
There was a problem hiding this comment.
The _TFilter name with underscore prefix conventionally signals an unused type parameter, but it's actually used (defaultFilter?: _TFilter). Why did you need it prefixed? Or what was the intention?
There was a problem hiding this comment.
had a weird issue with it not being registered first as used but now it seems like it works fine
=> adjusted it
| defaultSort, | ||
| defaultFilter, | ||
| filterKeys, | ||
| }: UsePaginatedTableOptions<TFilter, TSort>) { |
There was a problem hiding this comment.
The hook unconditionally creates sort and filter URL state for every consumer, even when a page doesn't use them. For example, DesiredState/Page.tsx and Notification/Center/Page.tsx don't destructure sort/setSort at all, and ResourceHistoryView.tsx and Orders/Page.tsx pass TFilter = undefined.
This means every page using this hook will always write sort (and filter) params into the URL, even on pages where those concepts don't apply. It may be worth making sort and filter opt-in (e.g. via flags in the options object), or at minimum documenting that unused return values still affect URL state.
There was a problem hiding this comment.
I don't think this is an actual issue unless i am missing something off course:
-
The default URL on pages like "DesiredState" looks exactly as expected.
Even though the useUrlStateWithSort and the useUrlStateWithFilter are being called, the handleUrlState is purely reactive and never writes to the URL?
Defaults are also serialized asundefinedso they're never written either. -
The URL state is cleared on navigation, so there's no risk of sort/filter params from going from one page into another right?
So there should not be any side effects on pages that don't use sort/filter?
i have put some comments on the hook to make it more clear let me know if this suffices for you.
There was a problem hiding this comment.
It's not really a behavior issue, the problem is forcing consumers to explicitly declare TFilter = undefined, or TSorting = undefined (I don't think we have this one in the codebase just yet), when they have no filter or sorting. It's semantically noisy.
I think we can get away with just updating the typing to something like this :
interface UsePaginatedTableOptions<TSort extends string = string, TFilter = undefined> { ... }
export function usePaginatedTable<TSort extends string = string, TFilter = undefined>(...) { ... }
(I inverted the order because the filtering is most often the one you need to specify when calling the hook.)
The sorting is nearly always a string, with the exception of the new resource view, but that's for a later stage.
I think you're right about the write to url, I wasn't entirely certain if the useUrlStateWithXXX wouldn't add undefined in the url when initialized.
Description
This was mainly to reduce boilerplate in all of those files.
#6754