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(mantine): export useUrlSyncedState and support hash router [ADUI-10457] #3992

Merged
merged 1 commit into from
Jan 8, 2025

Conversation

rphilippen
Copy link
Contributor

@rphilippen rphilippen commented Jan 8, 2025

Related ticket(s)

ADUI-10457

Proposed Changes

⚠️ The scope of these changes is a bit more than strictly required for the mentioned defect fix, since this fix was done as part of an effort to use useUrlSyncedState from Admin-UI.

The short summary:

  • Export useUrlSyncedState, so it can be used from Admin-UI too.
    (It appears to be the best way forward for Data Platform Foundation to fix another state + search parameters related issue).
  • Support the "hash router", that puts the page path inside the hash (#) section of the URL.
    Officially the 'search' part comes before the 'hash', but for page hashes there should be a "fake" search inside the hash path too. This makes sense when you realize that the whole reason for using the HashRouter is making sure you don't send the actual visited pages to the server (including the client-side search params).
  • Refactored the code to have behavior I think was "better" to make this hook public.

My team (Data Platform Foundation) has a defect which boils down to unfortunate (or just bad) state management related to search params. One of the issues is that we're tracking states in 3 different places, and we made the search parameters leading. Another issue is that we're not replacing the history like this hook is, so going back is like undo-ing your last filter/date range change. While this seemed like a nice feature when developing it, it is actually a dark pattern (back/forward buttons should be navigation, not undo/redo).

Long story short, I started prototyping replacing the state management we had with using the useUrlSyncedState hook. To do so I copied the hook to Admin-UI, and started using it. I ran into the same issue with the incorrect URL update, but fixed it differently as what was proposed in #3991. I basically kept using window.location.href, but just made the code sensitive to detect the hash route pattern (to my knowledge, it's always a # hash followed by a / forward slash). This keeps the benefits of the code always operating on the current URL, even though it doesn't have access to the useSearchParameters hook.

Arguably it is a bit cleaner to mutate through useSearchParameters, but also more cumbersome, so I will leave it to reviewers to judge if they're as confident in this approach as I am.

To make the hook more general purpose and hopefully also well behaved and performant, I changed some details as opposed to the previous version:

  • Make the optional sync parameter default to true (instead of false).
    To me this is the only logical choice; if you use useUrlSyncedState, the default behavior should be that it synchronizes. Otherwise you could just use setState.
    Of course the useTable hook is a bit of an exception, as it wants provide this feature as an option, and default to not syncing. So the useTable code had to be updated to always resolves the sync parameter to a boolean value, defaulting to false for undefined.
  • Only serialize the initialState once (provided it doesn't change), using useMemo.
  • Apply the changes caused by a serialize in a single "replaceState".
    This is a minor detail, but the previous version of the code would apply the state to the location per field that changed, instead of per state update.
  • Improved the TypeScript signatures a bit more, for example by using React signatures where we mimic setState.

Potential Breaking Changes

These changes fix the linked defect (usage with HashRouter), and adds the export of this hook (so it is an additive change). While there are some behavioral changes to the useUrlSyncedState hook, it was only internal before this PR, so there is no behavior breaking change.

Acceptance Criteria

  • The proposed changes are covered by unit tests
  • The potential breaking changes are clearly identified
  • README.md is adjusted to reflect the proposed changes (if relevant)

☝️ I didn't really see a README that should be updated, and did my best to add clear TSDoc. Still, please point out if there is a readme that should be updated.

@rphilippen rphilippen requested a review from a team as a code owner January 8, 2025 19:05
@rphilippen rphilippen requested review from gdostie and FelixBlaisThon and removed request for a team January 8, 2025 19:05
Copy link

github-actions bot commented Jan 8, 2025

@gdostie gdostie merged commit a10fbd0 into master Jan 8, 2025
6 checks passed
@gdostie gdostie deleted the feat/adui-10457-hash-state-support-and-export branch January 8, 2025 21:48
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