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

Build broken against React 18 #564

Closed
tombruijn opened this issue Apr 11, 2022 · 8 comments · Fixed by #567
Closed

Build broken against React 18 #564

tombruijn opened this issue Apr 11, 2022 · 8 comments · Fixed by #567
Assignees
Labels

Comments

@tombruijn
Copy link
Member

tombruijn commented Apr 11, 2022

The CI build is broken again react@latest. We should see what's broken and fix it.

https://appsignal.semaphoreci.com/workflows/ea3369ec-cbd0-491f-a087-23ac1c6c4566?pipeline_id=3ec8f69d-5655-458e-bc10-ff11e168a8a5

Blocks #563

@tombruijn tombruijn added the bug label Apr 11, 2022
@tombruijn
Copy link
Member Author

Not sure when this broke because there was no scheduled build set for the JavaScript repo. I've configured that now.

@tombruijn
Copy link
Member Author

tombruijn commented Apr 11, 2022

Looks like React version 18 is released and our package is locked at < 18

"react": ">= 16.8.6 < 18.0.0"

warning " > @appsignal/[email protected]" has incorrect peer dependency "react@>= 16.8.6 < 18.0.0". 00:02

I don't think that's the issue directly, probably that we don't support React 18 yet.

@luismiramirez
Copy link
Member

luismiramirez commented Apr 27, 2022

Tried to add it to the matrix after receiving the notification of the release of v18.1 here: 1fabeb3

I had to update the testing library to get rid of the first error described in this issue, and then it threw a new error:

https://appsignal.semaphoreci.com/jobs/95364096-c361-44ce-bbf2-c818301dbf7c

This requires further investigation cause how our ErrorBoundary works may need a change for v18

@unflxw unflxw changed the title Build broken against react@latest Build broken against React 18 Apr 27, 2022
@maltesa
Copy link

maltesa commented May 6, 2022

While this is open, I'm using a "custom" ErrorBoundary as a replacement. I'm not passing in the appsignal client as a prop. Apart from that, the API should be the same.

Maybe _errorInfo: ErrorInfo contains useful information that could be added to the span?

import { config } from '@/lib/config'
import Appsignal from '@appsignal/javascript'
import { Component, ErrorInfo, ReactNode } from 'react'

export const appsignal = new Appsignal({
  key: config.appsignalFrontendKey,
  revision: config.revision,
})

interface Props {
  action?: string
  children: ReactNode
  fallback?: (error: Error) => JSX.Element
  tags?: { [key: string]: string }
}

interface State {
  error?: Error
}

export class ErrorBoundary extends Component<Props, State> {
  public state: State = {
    error: undefined,
  }

  public static getDerivedStateFromError(error: Error): State {
    // Update state so the next render will show the fallback UI.
    return { error }
  }

  public componentDidCatch(error: Error, _errorInfo: ErrorInfo) {
    const { action, tags = {} } = this.props
    const span = appsignal.createSpan()

    span.setError(error).setTags({ framework: 'React', ...tags })

    if (action && action !== '') span.setAction(action)

    appsignal.send(span)
  }

  public render() {
    if (this.state.error) {
      return this.props.fallback ? this.props.fallback(this.state.error) : null
    }

    return this.props.children
  }
}

@tombruijn
Copy link
Member Author

The broken build is very annoying. We can disable the react 18 build for now while it's broken. And then pick up this issue.

@luismiramirez
Copy link
Member

luismiramirez commented May 11, 2022

Hi @maltesa!

Doesn't it work by default for you with React 18 unless you make that modification to componentDidCatch? It worked for us in our tests without changing anything.

With "that modification" I mean using the 2 arguments function.

@maltesa
Copy link

maltesa commented May 12, 2022

Just tried again with the ErrorBoundary from @appsignal/react and it works flawlessly as far as I can see. So no, “that modification” is not necessary.

@luismiramirez
Copy link
Member

Thanks for answering @maltesa!

I created an issue to cover your suggestion: #568.

You can keep track of the request from there.

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 a pull request may close this issue.

3 participants