✨ NextJS- add nextjs error boundary component#4352
✨ NextJS- add nextjs error boundary component#4352BeltranBulbarellaDD wants to merge 10 commits intomainfrom
Conversation
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage (details) 🔗 Commit SHA: 6d7f023 | Docs | Datadog PR Page | Was this helpful? React with 👍/👎 or give us feedback! |
Bundles Sizes Evolution
🚀 CPU PerformancePending... 🧠 Memory PerformancePending... |
There was a problem hiding this comment.
Since all these tests are the same for rum-react (except for the last one) should I remove them?
| "build:docs:json": "typedoc --logLevel Verbose --json ./docs.json", | ||
| "build:docs:html": "typedoc --out ./docs", | ||
| "pack": "yarn workspaces foreach --all --parallel --include '@datadog/*' exec yarn pack", | ||
| "pack": "yarn workspaces foreach --all --parallel --topological --include '@datadog/*' exec yarn pack", |
There was a problem hiding this comment.
This is required because rum-nextjs needs rum-react
There was a problem hiding this comment.
I'm not sure about this... rum depends on rum-code which depends on core and that was not an issue before.
| stdout: 'pipe' as const, | ||
| cwd: path.join(__dirname, '../apps/nextjs'), | ||
| command: isLocal ? 'yarn dev' : 'yarn start', | ||
| command: 'yarn start', |
There was a problem hiding this comment.
This is for 2 reasons:
- So that StrictMode does not double-fires in dev mode.
- The dev error overlay does not appear (ex)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d7f0239ab
ℹ️ 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".
| export { DatadogAppRouter } from '../domain/nextJSRouter/datadogAppRouter' | ||
| export { DatadogPagesRouter } from '../domain/nextJSRouter/datadogPagesRouter' | ||
| export { addNextjsError } from '../domain/error/addNextjsError' | ||
| export { ErrorBoundary } from '../domain/error/errorBoundary' |
There was a problem hiding this comment.
Avoid forcing rum-react on every rum-nextjs import
Re-exporting ErrorBoundary from the root entrypoint makes @datadog/browser-rum-react/internal a hard runtime dependency for all consumers, because both CJS and ESM need to resolve ../domain/error/errorBoundary when @datadog/browser-rum-nextjs is imported. That breaks existing integrations that only use DatadogAppRouter/addNextjsError and rely on @datadog/browser-rum-react being optional (as declared in peerDependenciesMeta), resulting in module-resolution failures unless they install rum-react.
Useful? React with 👍 / 👎.
| "build:docs:json": "typedoc --logLevel Verbose --json ./docs.json", | ||
| "build:docs:html": "typedoc --out ./docs", | ||
| "pack": "yarn workspaces foreach --all --parallel --include '@datadog/*' exec yarn pack", | ||
| "pack": "yarn workspaces foreach --all --parallel --topological --include '@datadog/*' exec yarn pack", |
There was a problem hiding this comment.
I'm not sure about this... rum depends on rum-code which depends on core and that was not an issue before.
| expect(customErrors[0].context).toMatchObject({ | ||
| framework: 'nextjs', | ||
| nextjs: { digest: expect.any(String) }, | ||
| }) |
There was a problem hiding this comment.
suggestion:
expect(errorEvent.error.component_stack).toBeDefined()There was a problem hiding this comment.
suggestion: this should not be needed. During development, the path points to the correct source, and for production builds the package.json is pointing to the right .d.ts file
| "@datadog/browser-rum-core": "6.31.0" | ||
| }, | ||
| "peerDependencies": { | ||
| "@datadog/browser-rum-react": "6.31.0", |
There was a problem hiding this comment.
suggestion: @datadog/browser-rum-react should be a direct dependencies, not a peer dep or a dev dep. Customers should not need to install the react integration themselves
There was a problem hiding this comment.
sounds like this as nothing to with this PR
Motivation
Adds React error boundary to NextJS pages router.
Changes
Adds
ErrorBoundary— a React error boundary component that wrapscreateErrorBoundaryfrom@datadog/browser-rum-react, pre-wired to calladdNextjsError.Exports
ErrorBoundaryFallbackandErrorBoundaryPropstypes alongside it.Also adds
@datadog/browser-rum-reactas an optional peer dependency ofrum-nextjs, and allows@datadog/browser-rum-react/internalthrough thedisallow-side-effectsESLint rule.Test instructions
Test app — new pages for both routers to exercise all error paths:
pages-router/error-test.tsxerror.tsxsegment boundary callingaddNextjsErrorerror.tsxcallingaddNextjsErrorglobal-error.tsxcallingaddNextjsErrorChecklist