♻️ React- Replace addEvent for AddError, Export Error Boundary#4317
♻️ React- Replace addEvent for AddError, Export Error Boundary#4317BeltranBulbarellaDD merged 5 commits intomainfrom
Conversation
BeltranBulbarellaDD
changed the title
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage (details) 🔗 Commit SHA: 8576067 | Docs | Datadog PR Page | Was this helpful? React with 👍/👎 or give us feedback! |
Bundles Sizes Evolution
🚀 CPU Performance
🧠 Memory Performance
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32bdcc7216
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const DatadogErrorBoundary: React.ComponentClass<ErrorBoundaryProps> = class DatadogErrorBoundary extends BaseErrorBoundary { | ||
| protected reportError(error: Error, errorInfo: ErrorInfo) { | ||
| reportError(error, errorInfo) | ||
| } | ||
| } | ||
|
|
||
| DatadogErrorBoundary.displayName = displayName | ||
|
|
||
| return DatadogErrorBoundary |
There was a problem hiding this comment.
💬 suggestion: We could remove some complexity by returning the class directly from here. What do you think?
| const DatadogErrorBoundary: React.ComponentClass<ErrorBoundaryProps> = class DatadogErrorBoundary extends BaseErrorBoundary { | |
| protected reportError(error: Error, errorInfo: ErrorInfo) { | |
| reportError(error, errorInfo) | |
| } | |
| } | |
| DatadogErrorBoundary.displayName = displayName | |
| return DatadogErrorBoundary | |
| export function createErrorBoundary( | |
| reportError: (error: Error, errorInfo: ErrorInfo) => void, | |
| displayName = 'ErrorBoundary' | |
| ) { | |
| const DatadogErrorBoundary: React.ComponentClass<ErrorBoundaryProps> = class extends React.Component< | |
| ErrorBoundaryProps, | |
| State | |
| > { | |
| constructor(props: ErrorBoundaryProps) { | |
| super(props) | |
| this.state = INITIAL_STATE | |
| } | |
| static getDerivedStateFromError(error: Error): State { | |
| return { didCatch: true, error } | |
| } | |
| resetError = () => { | |
| this.setState(INITIAL_STATE) | |
| } | |
| componentDidCatch(error: Error, errorInfo: ErrorInfo) { | |
| reportError(error, errorInfo) | |
| } | |
| render() { | |
| if (this.state.didCatch) { | |
| return React.createElement(this.props.fallback, { | |
| error: this.state.error, | |
| resetError: this.resetError, | |
| }) | |
| } | |
| return this.props.children | |
| } | |
| } | |
| DatadogErrorBoundary.displayName = displayName | |
| return DatadogErrorBoundary | |
| } |
There was a problem hiding this comment.
Hm I don't think it's possible as CallExpression can have side effects when the module is evaluated. Maybe move it in a function declaration?. This way createErrorBoundary(addReactError) can break tree-shaking. WDYT? Should we keep it?
There was a problem hiding this comment.
Let's remove that function. Export the BaseErrorBoundary class and extend it like you do below.
| const DatadogErrorBoundary: React.ComponentClass<ErrorBoundaryProps> = class DatadogErrorBoundary extends BaseErrorBoundary { | ||
| protected reportError(error: Error, errorInfo: ErrorInfo) { | ||
| reportError(error, errorInfo) | ||
| } | ||
| } | ||
|
|
||
| DatadogErrorBoundary.displayName = displayName | ||
|
|
||
| return DatadogErrorBoundary |
There was a problem hiding this comment.
Let's remove that function. Export the BaseErrorBoundary class and extend it like you do below.
|
@codex pls review |
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
| protected abstract reportError(error: Error, errorInfo: ErrorInfo): void | ||
| } | ||
|
|
||
| export function createErrorBoundary( |
There was a problem hiding this comment.
Relevant to the open discussion about exporting BaseErrorBoundary for direct extension: React explicitly recommends against component inheritance hierarchies — "we haven't found any use cases where we would recommend creating component inheritance hierarchies". Might be worth factoring that in when deciding which API to ship.
BeltranBulbarellaDD
deleted the
beltran.bulbarella/refactor_react_errors
branch
4 participants
Motivation
The
addReactErrorfunction was building and dispatching a raw RUM error event directly, duplicating logic already handled byaddErrorinrum-core.Additionally,
ErrorBoundarywas not composable, there was no way to create a customised error boundary that reports errors through a different mechanism (e.g. for the upcomingNext.jsintegration)The idea is to refactor
addReactErrorandErrorBoundarywithout changing functionality.Changes
reactPlugin.ts: replacedaddEventwithaddErroras the value passed toonRumStartsubscribers, react plugin no longer needs access to the raw event APIaddReactError.ts: simplified to delegate directly toaddError(), removing the manualcomputeRawError/ event constructionerrorBoundary.ts: introducedBaseErrorBoundaryabstract class andcreateErrorBoundary(reportError,displayName) factory, so error boundaries can be composed with any error-reporting callback;ErrorBoundarynow extendsBaseErrorBoundaryerror/index.ts: exportedcreateErrorBoundarysrc/entries/errorBoundary.tsentry point anderror-boundary/package.jsonfor the@datadog/browser-rum-react/error-boundarysubpath exportinitializeReactPlugintest helper and all affected specs accordinglyUsage of the changes in
Error Boundarycan be seen hereTest instructions
Old unit and e2e test should pass.
Added extra validation unit and e2e tests for the new
addErrorfunction.Checklist