chanzuckerberg / cryoet-data-portal

CryoET Data Portal
MIT License
16 stars 9 forks source link

Refactor query parameter state to jotai #879

Open codemonkey800 opened 1 month ago

codemonkey800 commented 1 month ago

A majority of our state lives in URL parameters accessible via the useSearchParams hook. While this works well because of its simplicity, it's not very performant nor maintainable in its current state.

Search parameter state usage is more or less scattered around the codebase and has no standardization around how it's managed, with the exception of useDownloadModalQueryParamState(). Using a state management solution would allow us to more easily organize our state.

Using search parameters as state also causes more unnecessary re-renders because calling setSearchParams() will result a re-render of the entire page rather than the components that are using that state. Furthermore, extra care is required when updating the parameters because successive calls to setSearchParams() are not batched, resulting in possibly additional re-renders. Moving our state into a state management library would allow us to set up a subscriber to update the URL parameters locally using history.replaceState() which would be more efficient

For example, here's how it's done on napari-hub:

/**
 * Updates the URL query parameters using the search form state.  On initial
 * load, some query parameters (page, sort type) will not have the query
 * parameters added (unless explicitly set by the user) because it looks nicer
 * to land on `https://napari-hub.org` instead of
 * `https://napari-hub.org/?sortType=recentlyUpdated&page=1`.
 *
 * @param initialLoad Whether this is the first time the page is loading.
 */
function updateQueryParameters({
  initialLoad,
  searchStore,
}: {
  initialLoad?: boolean;
  searchStore: PluginSearchStore;
}) {
  const url = new URL(window.location.href);
  const params = searchStore.getSearchParams({
    initialLoad,
    path: url.pathname + url.search,
  });

  url.search = params.toString();
  if (window.location.href !== url.href) {
    replaceUrlState(url);
  }
}

Because of this, we should consider migrating all of our state into jotai to reap the benefits in performance and maintainability.

codemonkey800 commented 1 month ago

hmm something interesting I found:

https://github.com/kentcdodds/kentcdodds.com/blob/ee9010414ec7f294eae07c959adbb66ec5ec8ddd/app/utils/misc.tsx#L303-L330

Kent C Dodds bypasses the re-rendering + re-executing loader by using window.history.replaceState instead of using useSearchParams() directly. this might useful if we want to try and fix the re-rendering issues without refactoring everything to global state

codemonkey800 commented 1 month ago

I might need to think about this more, this seems like it might be an anti-pattern in remix: https://remix.run/docs/en/main/discussion/state-management#how-remix-simplifies-state

codemonkey800 commented 1 month ago

see also: https://github.com/remix-run/react-router/discussions/9851