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

✨ Dynamic Reports: Implement server-side filtering hooks and example usage on Dependencies table and Issues table #839

Merged
merged 29 commits into from
Apr 28, 2023

Conversation

mturley
Copy link
Collaborator

@mturley mturley commented Apr 27, 2023

Follow-up to #829.
Part of #827. All that remains to close that issue is adding the drawer.

b1WMKwkA2w

  • Reorganizes files for better colocation of separated concerns:
    • Renames @app/shared/hooks/use-table-controls to table-controls
    • Creates subfolders for filtering, sorting and pagination under table-controls, each with:
      • use*State.ts, for state storage only (with React state and URL param variants of each)
      • getLocal*DerivedState.ts, for the logic specific to client-side filtering/sorting/pagination that was removed from each state hook
        • These have been renamed from useLocal*DerivedState because they were not calling any hooks, and it made sense to reserve the use naming for things that must conform to React's hook rules.
      • get*HubRequestParams.ts, for returning partial slices of the HubRequestParams object relevant to each concern from state and other args, and also contains serialize*RequestParamsForHub functions for converting each slice of HubRequestParams into the actual URLSearchParams used by the backend.
      • get*Props.ts, for PatternFly-specific props we can derive for each concern
      • In the pagination case, usePaginationEffects.ts, which contains the special useEffect case we needed for pagination only (making sure we return the user to a valid page if the results change in a refetch and they end up in an invalid place). If we end up needing useEffect calls for the other concerns, we could add a similar hook for them.
    • Implements the filtering version of all of the above files/functions. See getFilterHubRequestParams.ts for most of the new complexity.
    • Renames useTableControlState to useLocalTableControlState to make it more clear that it is only for use when working with local client-side filtering/sorting/pagination.
  • Moves the contents of @app/utils/hub-request-utils.ts to @app/shared/hooks/table-controls/getHubRequestParams.ts because it is coupled to the table-controls concerns and its logic could be refactored out into the separate functions for each concern described above.
  • getHubRequestParams.ts now simply calls all the get*HubRequestParams and serialize*RequestParamsForHub functions that have been separated per slice, and combines them.
  • The separation between converting things to HubRequestParams and converting that to URLSearchParams is useful because HubRequestParams is retained on response objects for inspection, and it provides a structured way to think about the hub's params without worrying about the actual serialization of them.
  • Refactors the legacy state hooks to reuse the new separated helpers and prevent duplication:
    • useLegacyFilterState was introduced to replace the existing usages of useFilterState which rely on retaining the derived state logic (client-side filtering). I was able to fully reuse useFilterState and getLocalFilterDerivedState within it so we don't have any duplicated logic, just a shim for the old calls.
    • Similarly, I realized I could refactor useLegacyPaginationState to reuse usePaginationState, usePaginationEffects, getLocalPaginationDerivedState and getPaginationProps so we no longer have any duplicated logic. Any bug fixes to filtering and pagination will apply to old usages of them as well.
    • I did not make a similar refactor to useLegacySortState because its paradigm is too different to easily share code (it relies on column indexes instead of columnKeys).
  • Updates useUrlParams to support removing params from the URL when they are empty (represented by a null value in the deserialized param). This is used so that when filters are missing or cleared, we don't have an extra filters param in the URL with nothing in it, and we can simplify logic that checks if there are no filters.
  • Adds a new generic type param TFilterCategoryKey to the FilterCategory types and useFilterState / useLegacyFilterState hooks. This allows the filterValues object to be strictly typed (as Record<TFilterCategoryKey, FilterValue> instead of Record<string, FilterValue>) and support autocomplete, because its keys are known (they come from the key property of each filter category object).
    • NOTE: This required updating everywhere we use useLegacyFilterState across the UI. Also, in each of the old calls, since filterCategories must be initialized in a variable outside the state hooks and FilterToolbar where the TItem type is known, it must be passed the type params explicitly. TypeScript does not currently support inference of some generic type params when others are passed in, you must pass in either none of them or all of them. So this means each of those calls must repeat all the key values from the filterCategories as a string union type. This is something we could fix if TypeScript incorporates a new feature in development: Perform partial inference with partially filled type parameter lists microsoft/TypeScript#54047

mturley added 22 commits April 28, 2023 12:06
Signed-off-by: Mike Turley <[email protected]>
…tate, start on useFilterState

Signed-off-by: Mike Turley <[email protected]>
Signed-off-by: Mike Turley <[email protected]>
Signed-off-by: Mike Turley <[email protected]>
Signed-off-by: Mike Turley <[email protected]>
…he filter/sort/pagination folders

Signed-off-by: Mike Turley <[email protected]>
Signed-off-by: Mike Turley <[email protected]>
Signed-off-by: Mike Turley <[email protected]>
Signed-off-by: Mike Turley <[email protected]>
Signed-off-by: Mike Turley <[email protected]>
Signed-off-by: Mike Turley <[email protected]>
Signed-off-by: Mike Turley <[email protected]>
@mturley mturley force-pushed the server-side-filters branch from 97e8d5d to 36c907f Compare April 28, 2023 16:06
Signed-off-by: Mike Turley <[email protected]>
@mturley mturley force-pushed the server-side-filters branch from 54452ee to 20a94eb Compare April 28, 2023 18:05
@mturley mturley changed the title ✨ Dynamic Reports: Implement server-side filtering hooks and example usage on Dependencies table ✨ Dynamic Reports: Implement server-side filtering hooks and example usage on Dependencies table and Issues table Apr 28, 2023
@mturley mturley marked this pull request as ready for review April 28, 2023 19:58
@ibolton336 ibolton336 merged commit 3ce0b35 into konveyor:main Apr 28, 2023
@mturley mturley deleted the server-side-filters branch May 1, 2023 14:50
mturley added a commit that referenced this pull request May 3, 2023
Just a couple little things I missed in #839.

---------

Signed-off-by: Mike Turley <[email protected]>
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

Successfully merging this pull request may close these issues.

2 participants