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

Setup root error boundary to gracefully handle all unhandled errors #104

Merged
merged 5 commits into from
Sep 15, 2023
Merged
Show file tree
Hide file tree
Changes from 3 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
14 changes: 13 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -62,15 +62,26 @@ yarn lint
yarn test
```

### Clean

Removes the `.next` and `node_modules` folders and runs `yarn install` to clean up the local workspace

```bash
yarn clean
```

### Take Screenshots

If you are making changes that impact the UI, ensure to test the change on different view ports. Run the following to auto capture screenshots when tests run. Note that this only runs when you run this command locally.

#### Take screenshots of home page

```bash
yarn capture-screenshots
```

#### Take screenshots of a specific page

```bash
PAGE_PATH=/commodities yarn capture-screenshots
```
Expand Down Expand Up @@ -116,4 +127,5 @@ yarn dev
- Docker

### 🌐 Useful Tools
- [ColorKit](https://colorkit.co/color-palette-generator/272f33-4e5d66-9BB9CB-cddce5-e6eef2/) - Color Palette Generator

- [ColorKit](https://colorkit.co/color-palette-generator/272f33-4e5d66-9BB9CB-cddce5-e6eef2/) - Color Palette Generator
34 changes: 34 additions & 0 deletions app/error.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
'use client';

import { Button, Center, Flex, Heading, Text } from '@chakra-ui/react';

export default function Error({
error,
reset,
}: {
error: Error;
reset: () => void;
}) {
return (
<Center width="100%">
<Flex
flexDirection="column"
alignItems="center"
gap="24px"
maxWidth="1500px"
nitishdhar marked this conversation as resolved.
Show resolved Hide resolved
>
<Heading
as="h1"
size={{ base: 'md', md: 'lg', lg: 'lg' }}
marginX={{ base: 'auto', md: '0', lg: '0' }}
>
Something went wrong!
</Heading>
<Text as="samp">{error.message}</Text>
<Button type="button" variant="outline" onClick={() => reset()}>
Try again
</Button>
</Flex>
</Center>
);
}
7 changes: 3 additions & 4 deletions app/not-found.tsx
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
'use client';

import NextLink from 'next/link';
import { Link, Center, Flex, Heading } from '@chakra-ui/react';
import GetColor from '@/app/_hooks/colorSelector';
import { Button, Center, Flex, Heading } from '@chakra-ui/react';

export default function NotFound() {
return (
Expand All @@ -21,9 +20,9 @@ export default function NotFound() {
404 - Page Not Found
</Heading>
<NextLink href="/" passHref>
<Link color={GetColor('textSelected')} href="#">
<Button type="button" variant="outline">
Go back Home
</Link>
</Button>
</NextLink>
</Flex>
</Center>
Expand Down
7 changes: 7 additions & 0 deletions app/playground/client-error/page.client.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
'use client';

const PageClient = () => {
throw new Error('This is a simulated client rendering error.');
};

export default PageClient;
8 changes: 8 additions & 0 deletions app/playground/client-error/page.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import PageClient from './page.client';

// Avoid statically building this page during build.
export const dynamic = 'force-dynamic';

export default async function Page() {
return <PageClient />;
}
aslink87 marked this conversation as resolved.
Show resolved Hide resolved
4 changes: 3 additions & 1 deletion app/playground/page.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import {
} from './_api';
import { ICommodity, IPost } from '../_types';

// Force dynamic fetching during runtime for playground
// Avoid statically building this page during build.
Copy link
Member

Choose a reason for hiding this comment

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

Could you explain the necessity of this please at the page.tsx level?

At this level I don't foresee us fetching anything that we don't want cached such as a list of commodity names that don't really change.

From what I understand NextJS will automatically switch these pages to dynamically rendered if uncached data is detected. It seems to go against the premise of caching if we were to force these routes to be dynamic.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had removed it in one of the commits in this PR based on the same understanding that it should not make these fetch calls during build but for some reason it was still making these calls during the github actions build. Have added this for now to fix that issue until I look into what's causing this to make these calls. However, it is needed in the other server-error scenario because I am not doing anything there for nextJs to detect that it should be done dynamically.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I also found this useful implementation to simulate github actions when needed, more details on the purpose and intent - #112. Will implement this to help with debugging these issues.

Copy link
Member

Choose a reason for hiding this comment

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

on the same understanding that it should not make these fetch calls during build

The calls that populate cached data at the page.tsx level should happen at build (atm we only do this to fetch an updated list of possible commodities). The data is required by the children. But you're probably talking specifically about the playground route, where you're right...though it's an exception to use 'no-cache' in our app.

From what I'm understanding here, there's a build error w/o this dynamic flag that occurs in the playground route because the server fetches are set to no-cache.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated this to use cache config.

Copy link
Collaborator Author

@nitishdhar nitishdhar Sep 14, 2023

Choose a reason for hiding this comment

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

It still fails - https://github.com/ed-pilots-network/frontend/actions/runs/6192011962/job/16811315676. I will debug this once I setup act (#112) for our package so I can do this locally. For now, have reverted back to forcing dynamic rendering.

export const dynamic = 'force-dynamic';

export const metadata: Metadata = {
Expand All @@ -23,12 +23,14 @@ export default async function Page() {
posts = await getPostsForCategoryFromMockApi();
} catch (error) {
console.error('Failed to fetch posts data:', error);
throw error;
}

try {
commodity = await getCommodityByNameFromStagingApi('Wine');
} catch (error) {
console.error('Failed to fetch commodity data:', error);
throw error;
}

return <PageClient posts={posts} commodity={commodity} />;
Expand Down
6 changes: 6 additions & 0 deletions app/playground/server-error/page.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// Avoid statically building this page during build.
export const dynamic = 'force-dynamic';

export default async function Page() {
throw new Error('This is a simulated server rendering error.');
}
22 changes: 22 additions & 0 deletions tests/playwright/playground/error-boundary.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import { test, expect } from '@playwright/test';

test('server rendering errors should get caught and gracefully render error', async ({
page,
}) => {
await page.goto('/playground/server-error');
await expect(page.locator('h1')).toContainText('Something went wrong!');
// Error messages for server rendering get replaced for security during builds
await expect(page.locator('samp')).toContainText(
'An error occurred in the Server Components render.',
);
});

test('client rendering errors should get caught and gracefully render error', async ({
page,
}) => {
await page.goto('/playground/client-error');
await expect(page.locator('h1')).toContainText('Something went wrong!');
await expect(page.locator('samp')).toContainText(
'This is a simulated client rendering error',
);
});