⚗ NextJS- add nextjs error boundary component#4352
⚗ NextJS- add nextjs error boundary component#4352BeltranBulbarellaDD merged 21 commits intomainfrom
Conversation
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage (details) 🔗 Commit SHA: 6aa5690 | Docs | Datadog PR Page | Was this helpful? React with 👍/👎 or give us feedback! |
Bundles Sizes Evolution
🚀 CPU Performance
🧠 Memory Performance
|
| "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.
There was a problem hiding this comment.
Hm let's discuss!
There was a problem hiding this comment.
Heeey so after a bit of investigation I think the issue comes from the integration packages having a "prepack" script. This script is called during yarn pack. It builds the package before packing it. This was useful when dogfooding the rum-react package in web-ui before publishing it (see #3182)
I think it makes sense and it should be added to all packages, not just integrations. By doing that, we won't need to run yarn build before running yarn run pack, the build will always be done during pack. As you noted, this requires to use the --topological flag when packing, similarly to the build script. I'll work on this in a separate PR. In the meantime, please add back the --topological option and remove the internal.d.ts file.
| 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".
| "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.
|
|
||
| import { disableJasmineUncaughtExceptionTracking, ignoreConsoleLogs } from '@datadog/browser-core/test' | ||
| import { appendComponent } from '../../../../rum-react/test/appendComponent' | ||
| import { initReactOldBrowsersSupport } from '../../../../rum-react/test/reactOldBrowsersSupport' |
There was a problem hiding this comment.
The test imports appendComponent and initReactOldBrowsersSupport via relative paths across package boundaries (../../../../rum-react/test/...). Is there a plan to extract these into a shared test utility, or is this the accepted pattern? It works, just looks fragile if packages ever move around.
There was a problem hiding this comment.
Agreed. Maybe we can expose them as shared utility, that would be better.
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