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

Install newest @tanstack/react-router #4384

Merged
merged 7 commits into from
Jul 24, 2024
Merged

Install newest @tanstack/react-router #4384

merged 7 commits into from
Jul 24, 2024

Conversation

apata
Copy link
Contributor

@apata apata commented Jul 23, 2024

Changes

This PR replaces react-router-dom with latest@tanstack/react-router. The reasoning is that we're using @tanstack/query to manage API response state in the application, and both packages depend on the same core logic package. We can expect these two to be complementing each other.

  • deprecates withRouter HOC
  • replaces PlausibleSearchParams with a stringifySearch and parseSearch method, performed automatically on location change by the new router
    • NB! components are operating with and changing the parsed object
    • NB! setting search param value to false used to be the way to remove the search param, but I wasn't able to keep this behavior, now setting the value to null is the way to do it
  • explicitly declares all app routes
    • makes navigating to each route use the route definition
  • refactors navigation links and buttons to new navigate API

Tests

  • Automated tests have been added
  • This PR does not require tests

Changelog

  • Entry has been added to changelog
  • This PR does not make a user-facing change

Documentation

  • Docs have been updated
  • This change does not need a documentation update

Dark mode

  • The UI has been tested both in dark and light mode
  • This PR does not change the UI

@apata apata added the preview label Jul 23, 2024
Copy link

Preview environment👷🏼‍♀️🏗️
PR-4384

@apata
Copy link
Contributor Author

apata commented Jul 23, 2024

How much should each component with a navigation action know about the query?

At the moment, their state is governed by the query, but they don't modify the query directly, they modify it by changing search params. I can see why, that's just what router utilities expect.

However, because search params =/= query, each component needs to be provided with a remapper of what search params to set to affect the query as expected. This is the questionable bit and I'm wondering if this remapping can be avoided.

Just to understand what each components responsibility should be, I created a small flow diagram.

---
title: Dashboard state and navigation
---
flowchart TB
    subgraph ENTRY
        direction TB
        BrowserArrives([Browser navigates to page]) -->
        ServerRenders[Server responds with HTML with site data encoded] -->
        SiteIsParsed[Site data is decoded from HTML] -->
        ReactAppMounts[React app mounts with the parsed site]
    end
    subgraph INITIAL
        direction LR
        RouterMounts([Initial browser path and search params are parsed]) --> 
        QueryContextMounts[Parsed search params and saved or hardcoded defaults are made available as `query` within the app]
    end
    subgraph ANY LIST LIFECYCLE
        direction TB
        AnyListMounts([Based on the browser path and `query`, a list component makes an API call and renders list items]) --> 
        AnyNavigationLinkMounts[A list item is able to set the browser path and new search params, with the new URL visible on hover] -- link clicked --> 
        RouterReloads[New browser path and search params are parsed] -->
        QueryContextReloads[Query context recalculates query based on the new search value]
        --> AnyListMounts
    end

    ENTRY --> INITIAL
    INITIAL --> AnyListMounts
Loading

@apata apata force-pushed the update-router-squashed branch from 605cb5b to 572f29e Compare July 23, 2024 14:02
@ukutaht
Copy link
Contributor

ukutaht commented Jul 24, 2024

How much should each component with a navigation action know about the query?
This is the questionable bit and I'm wondering if this remapping can be avoided.

Do you have a proposal for how it could be avoided?

In general I don't think it's a problem for components to know about query, especially now that we have the context for it.

@apata
Copy link
Contributor Author

apata commented Jul 24, 2024

How much should each component with a navigation action know about the query?
This is the questionable bit and I'm wondering if this remapping can be avoided.

Do you have a proposal for how it could be avoided?

In general I don't think it's a problem for components to know about query, especially now that we have the context for it.

As we discussed synchronously, it's OK that components are aware of the query. It's just that right now, they also simultaneously need to be aware of search params.

That's because we affect the query by using links that change search params. (This is good UX because users can easily open different selections of the dashboard in separate tabs if they need to.)

One potential improvement is to provide components with a utility that allows them to fill Link/navigate search property by knowing only what query change they need to affect:

function PageDrilldownLink({ page }) {
  const { set } = useQueryContext();
  return <Link to={rootRoute.to} search={set.pageFilter(page)}>{page.name}</Link>
}

That said, while search.filters ~~ query.filters && search.labels ~~ query.labels, we're pretty safe.

@apata apata merged commit 28cf3ff into master Jul 24, 2024
10 checks passed
@apata apata deleted the update-router-squashed branch July 24, 2024 12:14
zoldar added a commit that referenced this pull request Jul 26, 2024
zoldar added a commit that referenced this pull request Jul 26, 2024
@apata apata restored the update-router-squashed branch July 26, 2024 12:29
zoldar added a commit that referenced this pull request Jul 26, 2024
@apata apata mentioned this pull request Jul 31, 2024
8 tasks
@apata apata deleted the update-router-squashed branch January 23, 2025 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants