Handle invalid pagination parameters via exceptions #121

csillag · 2023-02-10T05:52:38Z

~~This depends on #124. (That's the first commit.) Please merge that one first, then rebase.~~

This is an alternate implementation of #119. (We only need one of them.)

This time we throw an error from the pagination hook, and apply React Router's errorElement feature to catch it.

Analysis:

The benefit is that the happy path and the error path are completely separated.
The drawback is that there is no TypeScript-level test to verify that the errorElement (or some other error boundary) is always there.

lukaw3d · 2023-02-10T13:37:40Z

src/app/components/Table/useSearchParamsPagination.ts

+    // This would be nicer, but this would also trigger react-error-overlay (besides our error boundary).
+    // We don't want that, so now, we are stuck with throwing a string, instead of a proper error.
+    // throw new AppError(AppErrors.InvalidPageNumber)


src/app/components/ErrorDisplay/index.tsx

lukaw3d · 2023-02-10T13:59:22Z

src/app/pages/BlockDetailPage/index.tsx

-      <TransactionsCard blockHeight={blockHeight} />
+      <ErrorBoundary>
+        <TransactionsCard blockHeight={blockHeight} />
+      </ErrorBoundary>


I find this more awkward than a full page error (but it should never happen so it doesn't matter)

Without With

I find the first one more awkward. Technically speaking the error only impacts the transaction list, so why shouldn't we seen the data about the block itself?

I see first one as giving clear instructions that URL is malformed. And second one like it could be an app bug

I can see your point. I wonder which viewpoint is more prevalent. I'm inclined to defer this question to @donouwens .

My suggestion would be to have a dedicated error state for those situations.
Like this: https://www.figma.com/file/ifCrok8cP5ymEYjMa2PIi9/MVP?node-id=2934%3A157297&t=CoRB18ePaVLjZtDq-1

@lukaw3d All other data is valid and relevant so why would we hide it? Is there a threat to it in any kind way?

First shot at approaching the design:

lukaw3d · 2023-02-10T14:05:29Z

src/routes.tsx

@@ -63,6 +66,7 @@ export const routes: RouteObject[] = [
      {
        path: `/${paraTime}/transactions`,
        element: <TransactionsPage />,
+        errorElement: <RoutingErrorPage />,


The drawback is that there is no TypeScript-level test to verify that the errorElement (or some other error boundary) is always there

Is there a way to add a top-level error handler?

Yes, in fact it's recommended by React Router. What should be the text? Just a generic unknown error?

Wherever it's needed. (On some other routes we already had `errorElements` declared.)

We have one at the root now, that should catch all.

csillag mentioned this pull request Feb 10, 2023

Handle invalid pagination parameters via explicit checking #119

Closed

csillag force-pushed the csillag/handle-invalid-page-numbers-with-exceptions branch 3 times, most recently from 4fda9d7 to 8c5aafe Compare February 10, 2023 06:43

csillag requested review from lukaw3d and buberdds February 10, 2023 07:06

csillag force-pushed the csillag/handle-invalid-page-numbers-with-exceptions branch 5 times, most recently from 00e11c0 to 1871cab Compare February 10, 2023 13:20

lukaw3d approved these changes Feb 10, 2023

View reviewed changes

lukaw3d reviewed Feb 10, 2023

View reviewed changes

csillag added 5 commits February 10, 2023 19:24

Add actual ErrorBoundary React component

1aea66f

errorFormatter: recognize string AppError codes

2b9da0b

Introduce new AppError sub-type for invalid page number

d2a5547

Add error elements to catch pagination errors

bf0af4d

Wherever it's needed. (On some other routes we already had `errorElements` declared.)

Tune design on partial errors

440020b

csillag force-pushed the csillag/handle-invalid-page-numbers-with-exceptions branch from 1871cab to 440020b Compare February 10, 2023 18:25

csillag added 3 commits February 10, 2023 19:51

Tune design on partial errors 2

bb49373

Remove spurious errorElements from routes.

f2c8a6d

We have one at the root now, that should catch all.

Don't define RoutingErrorDisplay as a separate component

38ed52a

csillag merged commit 02442f0 into master Feb 13, 2023

csillag deleted the csillag/handle-invalid-page-numbers-with-exceptions branch February 13, 2023 16:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle invalid pagination parameters via exceptions #121

Handle invalid pagination parameters via exceptions #121

csillag commented Feb 10, 2023 •

edited

Loading

lukaw3d Feb 10, 2023

lukaw3d Feb 10, 2023

csillag Feb 10, 2023

lukaw3d Feb 10, 2023

csillag Feb 10, 2023

donouwens Feb 10, 2023

csillag Feb 10, 2023

lukaw3d Feb 10, 2023

csillag Feb 10, 2023

Handle invalid pagination parameters via exceptions #121

Handle invalid pagination parameters via exceptions #121

Conversation

csillag commented Feb 10, 2023 • edited Loading

Analysis:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csillag commented Feb 10, 2023 •

edited

Loading