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

fix: reintroduce root suspense fallback UI, with logging #1261

Merged
merged 10 commits into from
Jan 31, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions .editorconfig
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# .editorconfig file
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[curious] Does this file need to be in here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't need to be, no. However, defining the .editorconfig file will help IDEs ensure consistent file formatting. For example, without this file, whenever I create a new file within VSCode, it defaults to using tabs (4 characters), requiring manual changes to change tabs to 2 spaces.

The .editorconfig files dictates to VSCode to set those params on new files on its own, for example.

# For more details, see https://editorconfig.org

root = true

[*]
indent_style = space
indent_size = 2
end_of_line = lf
charset = utf-8
trim_trailing_whitespace = true
insert_final_newline = true

[*.json]
quote_type = double

[*.md]
trim_trailing_whitespace = false
39 changes: 23 additions & 16 deletions docs/decisions/0014-rootLoader-revalidation.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,23 +2,21 @@

## Status

Accepted (January 2025)
Accepted/Revised (January 2025)

## Context

In this route-based application, the `rootLoader` is responsible for pre-fetching common queries needed across multiple routes. However, when navigating between sub-routes (e.g., from the Dashboard to the Search page), the `rootLoader` doesn’t re-execute. This becomes a problem when a component in the target route accesses query data with `useQuery` that hasn’t already been pre-fetched by a loader.
In this route-based application, the `rootLoader` is responsible for pre-fetching common queries needed across multiple routes. However, when navigating between sub-routes (e.g., from the Dashboard to the Search page), the `rootLoader` doesn’t re-execute on its own. This lack of revalidation becomes a problem when a component in the target route accesses query data with `useQuery` that hasn’t already been pre-fetched by a loader. This issue is particularly noticeable when navigating from a page route supported by the Backend-for-Frontend (BFF) / API Gateway to other page routes that continue to rely on the individual API queries instead of the BFF.

By default, this issue is masked due to the query client configuration where queries have `suspense: true`. In these cases, if a query hasn’t been pre-fetched in a route loader, the `useQuery` hook causes the component to suspend, triggering the `AppSuspenseFallback` component while the query resolves asynchronously. This fallback behavior hides the underlying problem, making it harder to identify when query data is being accessed without pre-fetching.
By default, this issue is largely masked due to the query client configuration where queries have `suspense: true`, by default. In these cases, if a query hasn’t been pre-fetched in a route loader, the `useQuery` hook causes the component to suspend, triggering the `AppSuspenseFallback` component while the query resolves asynchronously. This fallback behavior hides the underlying problem, making it harder to identify when query data is being accessed without pre-fetching in a route loader.

## Decision

To ensure data consistency and explicitly address the issue of missing pre-fetched queries, two key changes will be implemented:
While the parent suspense fallback in these scenarios prevents an error from surfacing to users while asychronous queries are resolving, the fallback behavior makes it less evident at the time of development to prevent the underlying issue altogether without improved observability.

### 1. Remove the Parent `Suspense` and `AppSuspenseFallback` Component
## Decision

The parent `Suspense` boundary and its fallback (`AppSuspenseFallback`) will be removed from the `App` component. By default, the query client configuration uses `suspense: true`, which masks the issue of missing pre-fetched query data by suspending components and displaying the fallback while queries resolve asynchronously. Removing this fallback ensures that missing query data triggers explicit errors instead of silently deferring to the fallback. This change improves the navigation experience and makes it easier to debug and fix data-loading issues during development.
To ensure data consistency and more directly address the issue of missing pre-fetched queries, two key changes will be implemented:

### 2. Implement `shouldRevalidate` on the `rootLoader`
### 1. Implement `shouldRevalidate` on the `rootLoader`

React Router by default only revalidates loaders after specific actions, such as form submissions or mutations. It does not automatically revalidate loaders during navigation between sibling routes under the same parent. To address this, the `shouldRevalidate` function will be implemented on the `rootLoader` route, with logic explicitly tailored to handle sub-route navigation.

Expand All @@ -29,26 +27,35 @@ The `shouldRevalidate` function will determine whether the loader should re-exec

This logic ensures that revalidation is scoped specifically to relevant sub-route transitions, avoiding unnecessary data fetching while ensuring consistent query cache availability.

### 2. Refactor parent `Suspense` component as Error Boundary

By only having a parent `Suspense` component to handle cases of suspended API queries at render time, there is no observability for when the suspense fallback is rendered, leaving minimal clues at development time that an issue exists.

Instead, the parent `Suspense` will be refactored first and foremost as a React error boundary (i.e., `AppErrorBoundary`) to catch any raised errors from its children. If an error is caught, `AppErrorBoundary` will determine whether the error is related to a missing suspense fallback. If so, the error will be logged, but the children will be re-rendered, wrapped with `Suspense` and its fallback UI. Other errors beyond suspense related issues will render the formatted `ErrorPage`, displaying the raised error message along with the CTA to "Try again", which triggers a full page refresh of the current route.

Letting the `Suspense`-related error raise and subsequently get caught by `AppErrorBoundary`, it affords the opportunity to log the issue via `logError` for improved observability without surfacing these errors in the UI to the user, beyond a brief secondary loading state.

To enable this approach, the `useNProgressLoader` hook that manages the state of the animated, horizontal loading progress bar, will be extended to accept `loaderOptions`, where callers of this hook can change its behavior as needed. In this case, the `AppErrorBoundary`'s conditional rendering of a `Suspense` fallback utilizing the `useNProgressLoader` hook, the conditions that determines when the loader completes may already be truthy. Given this, when the `Suspense` fallback is rendered within `AppErrorBoundary` with `loaderOptions.shouldCompleteBeforeUnmount: false`, the animated loader bar will only complete (i.e., `nprogress.done()`) once the `Suspense` fallback component unmounts.

### Benefits of this Approach

* Queries required by UI components in the target route are always pre-fetched and cached before rendering.
* Developers can easily identify and address missing pre-fetches through explicit errors instead of relying on silent fallback behavior.
* Data consistency is maintained across sibling route transitions, particularly during the incremental migration to a Backend-for-Frontend (BFF) or API Gateway architecture.
* Queries required by UI components in the target route are more likely to be pre-fetched and cached prior to rendering by ensuring the `rootLoader` is re-executed upon sub-route navigation, particularly during the incremental migration to a Backend-for-Frontend (BFF) / API Gateway architecture, while continuing to prevent `Suspense`-error states from displaying in the UI.
* Developers can more easily identify and address missing pre-fetches through explicitly logged errors instead of only relying on silent fallback behavior.

## Consequences

### Positive Outcomes

* **Reliable Query Data Availability:** Ensures that all required query data is pre-fetched and cached during sub-route transitions, preventing issues where `useQuery` tries to access missing or stale data.
* **Explicit Error Handling:** Removing the `AppSuspenseFallback` ensures that missing pre-fetched data triggers explicit errors instead of being masked by the fallback. This simplifies debugging and leads to better long-term reliability.
* **Explicit Error Handling:** Introducing error logging within the `AppSuspenseFallback` ensures that missing pre-fetched data triggers explicit errors instead of solely being masked by the fallback. This simplifies debugging, leading to better long-term reliability, while preventing related errors from surfacing to users.
* **Scoped and Efficient Revalidation:** The `shouldRevalidate` function selectively targets relevant sub-route transitions. Combined with `ensureQueryData`, it prevents redundant API requests for data that remains fresh, minimizing performance impact.
* **Improved Developer Workflow:** By exposing missing pre-fetches, the approach facilitates early identification and resolution of query-related issues, reducing risks of data inconsistencies.
* **Improved Developer Workflow:** By exposing missing pre-fetches, the approach strives to facilitate earlier identification and resolution of query-related issues, reducing risks of data inconsistencies.

### Negative Outcomes

* **Initial Debugging Effort:** Removing the fallback may reveal existing cases where query data was not pre-fetched. This change could introduce additional debugging during development as missing pre-fetches surface as explicit errors. However, addressing these issues early ensures long-term consistency in data-loading strategies by enforcing a robust pattern of pre-fetching query data in route loaders.
* **Initial Debugging Effort:** Keeping the suspense fallback may continue to obscure existing cases where query data was not pre-fetched in a route loader, for example, if issues with the loading UX and/or additional logs are not noticed during development. That said, the additional logs within `AppSuspenseFallback` should help surface such cases for resolution, while keeping the app functional for users.
* **Small Overhead for Non-Fresh Data:** Revalidating the `rootLoader` during sub-route navigation may result in occasional additional backend calls if data is not fresh. However, these calls are scoped and optimized, ensuring minimal impact on performance.

## Alternatives Considered

* Keeping the parent `Suspense` and its fallback component (`AppSuspenseFallback`) in the `App` component would allow the application to handle missing pre-fetches silently, suspending components and showing a secondary loading state during navigation. While this approach mitigates user-facing errors, it obscures data-loading inconsistencies and makes debugging more difficult. Additionally, it risks subtle issues in route navigation where query data dependencies are unclear. By adopting the chosen approach, explicit error handling ensures better enforcement of consistent and predictable data-loading practices.
* Removing the parent `Suspense` and its fallback component (`AppSuspenseFallback`) in the `App` component altogether would prevent the application to handle missing query pre-fetches silently, where suspended components would no longer have a fallback UI, raising an error in the UI. While surfacing the error in the UI to users helps developers more quickly identify affected routes and code paths, we would rather prevent the error from appearing in the UI, prioritizing a functional UX, while having better observability regarding when the `AppSuspenseFallback` component is rendered.
Loading