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(error): add global error pages with logging #1681

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

SelmaBergstrand
Copy link
Collaborator

@SelmaBergstrand SelmaBergstrand commented Oct 15, 2024

Lagt til ny error-side som skal catche og logge feil, både i pages og i app-router. Har i tillegg lagt til en custom 404-error-side (som ikke ble fanget opp av de andre error-fangerne).
Ser sånn ut for feil:
Screenshot 2024-10-16 at 11 37 44
for 404:
Screenshot 2024-10-16 at 11 38 21

@SelmaBergstrand
Copy link
Collaborator Author

OBS: global-error.tsx funker visst ikke alltid, det ser ut som en kjent Next bug (vercel/next.js#55462). I tillegg er pages sin error handler (_error) satt til å kun funke i production og ikke når man kjører opp ting med dev, så får ikke testa den enda

@SelmaBergstrand SelmaBergstrand changed the title Add global error handling feat(error): add global error pages with logging Oct 15, 2024
@SelmaBergstrand SelmaBergstrand marked this pull request as ready for review October 16, 2024 09:38
@emilielr
Copy link
Collaborator

Har du fått til å trigge global-error?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Jeg får aldri til å trigge denne siden - det ser ut som den faller tilbake på not-found.tsx i app-routeren. Muligens vi ikke trenger denne siden i det hele tatt?

Copy link
Collaborator

@emilielr emilielr Oct 18, 2024

Choose a reason for hiding this comment

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

Tror det kommer av dette i getServerSideProps() i [id].tsx:

  if (!board) {
        return {
            notFound: true,
        }
    }

Skader for så vidt ikke å ha den der, men unødvendig om den aldri trigges 🗑️ Ser også at board-objektet vi henter ut her aldri vil være undefined heller, så dette vil i utgangspunktet aldri trigges. Burde kanskje sett på hvordan man henter ut et board

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Har ikke fått til å trigge den jeg heller. Tanken er at global-error liksom er siste nivå av error catching, og kan feks catche feil som skjer i RootLayout (for disse greier visst ikke error.tsx å fange). Prøvde å trigge en der ved å throwe feil i navbaren men da får jeg bare stack trace som vanlig.

Copy link
Collaborator

@emilielr emilielr Oct 18, 2024

Choose a reason for hiding this comment

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

Det viser seg at den defaulter tilbake på not-found.tsx hvis man har både app og pages router ser det ut som: vercel/next.js#58945 (comment). Vi må uansett se på at board-objektet aldri er undefined. Nå returnerer vi hvilken som helst ID fordi vi ikke sjekker om objektet finnes i firebase først

Copy link
Collaborator

Choose a reason for hiding this comment

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

Og ja, global-error.tsx må man ha 👯

stacktrace: error.stack,
message: error.message,
url: window.location.href,
})
Copy link
Collaborator

@emilielr emilielr Oct 18, 2024

Choose a reason for hiding this comment

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

Skal vi legge til at vi logger digest også? Om en error ser på server-siden, så vil denne hjelpe oss med å finne ut va den eksakte feilmeldingen var: https://nextjs.org/docs/app/building-your-application/routing/error-handling#handling-server-errors

cause: error.cause,
stacktrace: error.stack,
message: error.message,
url: window.location.href,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Samme som over med digest her?

import { logger } from 'utils/logger'
import BeaverIllustration from 'assets/illustrations/Beaver.png'

const log = logger.child({ module: 'appLevelErrorHandler' })
Copy link
Collaborator

Choose a reason for hiding this comment

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

Kalle den kun appErrorHandler?

import BeaverIllustration from 'assets/illustrations/Beaver.png'
import { NextPageContext } from 'next'

const log = logger.child({ module: 'pagesLevelErrorHandler' })
Copy link
Collaborator

Choose a reason for hiding this comment

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

pagesErrorHandler?

Copy link
Collaborator

@emilielr emilielr left a comment

Choose a reason for hiding this comment

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

Jeg har lagt ved noen kommentarer 😸 Har vi gjort oss opp noen formening om hva som er lurt å logge når en error i appen skjer?

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