-
Notifications
You must be signed in to change notification settings - Fork 26.3k
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
_error.js getInitialProps is not called for an error thrown at the top level of a module #8592
Comments
But I could see the log from |
See vercel/next.js#8592 (comment). Since we were missing a link to the favicon, it caused the browser's request to /favicon.ico to result in a 404, which triggers a rendering of _error.js on the server side, which is unrelated to the bug this repo is demonstrating
Sorry, I can see how I wasn't specific enough. The goal is to get I've updated the repo to add a link to the favicon in |
Thanks @WestonThayer , now the console is totally silent. |
Just noting that this still repro's with |
Trying to use @WestonThayer 's workaround with Typescript results in an error:
Error:
What would be the right types to use in this case? |
@fredrik-sogaard it's not a publicly exposed part of AppProps, thus my workaround is taking advantage of non-public parts of Next.js's API and should be considered brittle. But, it works for now. I'm just casting to |
Slightly off topic but I hope you guys can help - this is driving me nuts. I've added the code from If I build locally and start, the server errors are logged. When I push and the project is built on Vercel, no server side errors logged to Sentry. I checked that the Sentry DSN is correct by printing it to console. The sourcemaps are uploaded also. Any ideas? EDIT: I deployed the example verbatim and server-side error tracking doesn't work there either. 😕 |
@georgiosd sounds like something changed since I last worked with it, but I do wonder if it's simply that #8685 started impacting more situations. To debug, try calling |
@WestonThayer How sad. I'm gonna try that but need to make some progress with the project first. Will report what I find. Mean time I opened another issue #14758 - I hope the Vercel folks will give a hand. |
Hey guys, any news about this? I'm having the same issue |
Same issue here, and it appears that the workaround of passing Edit: I was able to resolve my issue with this solution: i18next/next-i18next#615 (comment) |
Hi @WestonThayer I am stuck on trying to write a test for this in my app to check if can correctly catch all errors. In your sample repo, can you please explain more precisely what error will be produced? If you were to rewrite with an explicit import React from "react";
import Head from "next/head";
if (typeof window !== "undefined") {
throw new Error(
"Error thrown during top-level page rendering client-side (if you see this message in the ErrorBoundary, everything works as expected)."
);
}
const Home = () => {
return (
<div>Hello</div>
</div>
);
};
export default Home; I can't catch this properly using _error when opening this page but maybe I am doing smth wrong. Edit: I think I got it: Edit2: it seems to work but I have a discrepancy between SSR and client-render with this example. Server-side, I get a loader (which is expected when there is no failure). Client side, I get the error message. Result is kinda funny: my error turns around because there's a mix between the circular progress loader (from the server-render) and the error render client-side. |
@eric-burel sounds like you figured it out, building production is the way to test it. Is the progress loader something you added? Your example page jsx above should cause Next.js to build a static page, it shouldn't need to SSR that. |
Yeah I mean the static render, consequence is the same, you get a mismatch between the data sent by the server (the loader) and the client render (the error). There is a difference since the client is failing, which leads to hydratation issue. So the proposed fix might need a |
I've been working around this for the past few days and what I could see is that : ssr test 1 & 2 and client tests 2, 4 and 5 of the |
I just repro'd this bug on "dependencies": {
"next": "10.2.0",
"react": "17.0.2",
"react-dom": "17.0.2"
} And can confirm it still occurs. The workaround still works as well. Note that it does not cause I haven't had a chance to look at the |
I saw this and how it was handled but unfortunately it does not seem like err is passed on my end even though I made something like this in my _app : // some import goes here
sentryInit();
interface WorkaroundAppProps extends AppProps {
err: any;
}
export default function MyApp({ Component, pageProps, err }: WorkaroundAppProps): JSX.Element {
// some code goes here
return(
// some code goes here too
<Component {...pageProps} err={err} />
// some over code
}
}); what I do not understand also is almost no client side error is passed to Sentry. |
@antkougblenou It might be unrelated, but 10 days ago Sentry has released official npm package to support Next.js projects and it might help with the setup. I was in the same scenario as you, building with a custom implementation of Sentry for next.js but found that it's not worthy.
|
Unfortunately @kachar I am well aware of that package, but since the integration of sentry will be in a production project I cannot use a package that is interacting with experimental packages under the hood. For anyone out there that is fine with this @sentry/nextjs is indeed a good alternative. |
I don't know if the issue is being fixed or any workaround has been discussed. Anyway:
I guess it is related to the issue. So my current solution is to have a |
BTW, if anybody struggles with adding TypeScript to that workaround, here is a good example:
|
For a long time, the `_error.js` page we provide `@sentry/nextjs` users contained a workaround for vercel/next.js#8592. When most of the code in `_error.js` was moved into `captureUnderscoreErrorException`, parts of the workaround came along for the ride. Now that that issue has been fixed, the workaround is no longer necessary. This removes as many of the workaround's vestiges in `captureUnderscoreErrorException` as possible. (We can't remove them all, because the workaround existed in user code rather than ours, and we can't stop people from continuing to use it. This fixes things so that if they do that, it'll just bail gracefully.)
* fix(remix): Use cjs for main entry point (#5352) * ref(tracing): Only add `user_id` to DSC if `sendDefaultPii` is `true` (#5344) Add the check if `sendDefaultPii` is set to `true` before adding the `user_id` when populating DSC. Additionally, add some tests to check for the changed behaviour. Conforms with Sentry Dev Spec for handling sensitive data in DSC: https://develop.sentry.dev/sdk/performance/dynamic-sampling-context/#handling-sensitive-data-and-pii * docs(remix): Add missing comma in README (#5353) * fix(remix): Sourcemaps upload script is missing in the tarball (#5356) Add the sourcemaps upload script to the tarball, which required the following changes: * Add a package-specific `prepack.ts` script which is invoked by the main `prepack.ts` script. This is similar to how we do it with the Gatsby SDK where - just as with Remix - we need to copy over additional files to the `build` directory that are not part of our other packages. The newly added files are: * `scripts/sentry-upload-sourcemaps.js` * `scripts/createRelease.js` * Add a package-specific `.npmignore` which extends the root level `.npmignore` file to not ignore the `scripts` directory in the `build` directory. * docs(remix): Adjust wording in Remix README (#5357) * meta: Add CHANGELOG for 7.5.0 (#5354) Co-authored-by: Lukas Stracke <lukas@stracke.co.at> * release: 7.5.0 * ref(tracing): Remove transaction name and user_id from DSC (#5363) This patch temporarily removes the `user_id` and `transaction` (name) fields from the dynamic sampling context, meaning they will no longer propagated with outgoing requests via the baggage Http header or sent to sentry via the `trace` envelope header. We're taking this temporary measure to ensure that for the moment PII is not sent to third parties. * meta: Add changelog for 7.5.1 (#5372) * release: 7.5.1 * fix(remix): Move hook checks inside the wrapper component. (#5371) `withSentryRouteTracing` is called on `root.tsx` which runs before `entry.client.tsx` (where `Sentry.init` for browser is called and `remixRouterInstrumentation` is assigned). The check if the provided hooks are available was failing silently, and causing `withSentryRouteTracing` to early return the `App` unwrapped. This PR ensures that validation runs on the client side, after `entry.client.tsx`. * fix(remix): Strip query params from transaction names (#5368) * feat(tracing): Add transaction source field (#5367) This patch adds `source` information to `Transaction`, which is typed by `TransactionSource` in `@sentry/tracing`. This helps track how the name of a transaction was determined, which will be used by the server for server-side controls. For now, we are placing the `source` field under transaction metadata. In the future, we can move this up into a top level API (an argument to `startTransaction` or `transaction.setSource`) if needed, but this should be fine to get us started. For next steps, after this patch gets merged, we will start going through various routing instrumentation frameworks and adding transaction source. * build: Build with frozen lockfile (#5348) * fix(remix): Make peer deps less restrictive (#5369) * ref(build): Reenable lambda layer release in craft (#5207) * ref(tracing): Add transaction source to default router (#5386) * ref(react): Add source to react-router-v3 (#5377) * ref(angular): Add transaction source for Angular Router (#5382) Add the transaction name source annotation to the Angular SDK. Since the Angular SDK currently does not parameterize URLs, we only assign `'url'` as the transaction name source. We're revisiting parameterization in Angular in a follow up PR. Additionally, add a two tests to cover this change (might be more relevant once we add parameterization). * feat(vue): Add transaction source to VueRouter instrumentation (#5381) * ref(react): Add transaction source for react router v6 (#5385) * ref(react): Add transaction source for react router v4/v5 (#5384) * feat(remix): Wrap root with `ErrorBoundary`. (#5365) Wraps the Remix root with `ErrorBoundary` while wrapping it with the router instrumentation. * fix(remix): Wrap `handleDocumentRequest` functions. (#5387) We're currently wrapping `action` and `loader` functions of Remix routes for tracing and error capturing. When testing the case in #5362, I realized the `render` function of a SSR component in Remix has another handler [`handleDocumentRequest`](https://github.com/remix-run/remix/blob/b928040061890a6ef0270abdb5b1201638f0dd00/packages/remix-server-runtime/server.ts#L174) which [doesn't re-throw internal errors](https://github.com/remix-run/remix/blob/main/packages/remix-server-runtime/server.ts#L502-L507) so we can't catch them in `wrapRequestHandler`. Also added a tracing span for `handleDocumentRequest`. Co-authored-by: Abhijeet Prasad <aprasad@sentry.io> * ref(tracing): Include transaction in DSC if transaction source is not an unparameterized URL (#5392) This patch re-introduces the `transaction` field in the Dynamic Sampling Context (DSC). However, its presence is now determined by the [transaction source](https://develop.sentry.dev/sdk/event-payloads/transaction/#transaction-annotations) which was introduced in #5367. As of this we we add the `transaction` field back, if the source indicates that the transaction name is not an unparameterized URL (meaning, the source is set and it is not `url`). Additionally, the PR (once again) adjusts our unit and integration tests to reflect this change. Repurposed some DSC<=>`sendDefaultPii` tests that we previously skipped to now cover the transaction<=>transaction source dependence. Did some cleanup of commented out old code and explanations that no longer apply. Remove he `'unknown'` field from the `TransactionSource` type because it is only used by Relay and SDKs shouldn't set it. * feat(nextjs): Record transaction name source when creating transactions (#5391) This adds information about the source of the transaction name to all transactions created by the nextjs SDK. * ref(serverless): Add transaction source (#5394) * feat(tracing): Record transaction name source when name set directly (#5396) This adds transaction name source to transaction metadata in various situations in which the name is set directly: starting a manual transaction, assigning to the `name` property of a transaction, calling a transaction's `setName` method, and changing the name using `beforeNavigate`. In order to distinguish manually-created transactions from automatically-created ones, all internal uses of `startTransaction` have been changed so that they are happening directly on the hub, rather than through the top-level `Sentry.startTransaction()` wrapper. * meta: 7.6.0 CHANGELOG (#5397) * release: 7.6.0 * fix(remix): Add `documentRequest` function name. (#5404) * build: Upgrade prettier to 2.7.1 (#5395) * ref(remix): Add transaction source (#5398) * fix(remix): Skip capturing `ok` responses as errors. (#5405) [Remix supports throwing responses from `loader` and `action` functions for its internal `CatchBoundary`](https://remix.run/docs/en/v1/api/conventions#throwing-responses-in-loaders). They are [catched on the caller level](https://github.com/remix-run/remix/blob/7688da5c75190a2e29496c78721456d6e12e3abe/packages/remix-server-runtime/data.ts#L41-L49), but as we wrap the callees, they are registered as exceptions in the SDK. Being http responses, they end up like `{size: 0}`, which is not meaningful. This PR skips exception capturing for responses that are not `4xx` or `5xx`. * chore: Use correct function call in migration docs (#5414) * fix(core): Add `sentry_client` to auth headers (#5413) This adds `sentry_client` to the auth headers* we send with every envelope request, as described [in the develop docs](https://develop.sentry.dev/sdk/overview/#authentication). In order to have access to the SDK metadata, the full `ClientOptions` object is now passed to `getEnvelopeEndpointWithUrlEncodedAuth`. Despite the fact that this is really an internal function, it's exported, so in order to keep everything backwards-compatible, for the moment it will also accept a string as the second argument, as it has in the past. Finally, all of our internal uses of the function have been switched to passing `options`, and there's a `TODO` in place so that we remember to remove the backwards compatibility in v8. Note that this change doesn't affect anyone using a tunnel, as no auth headers are sent in that case, in order to better cloak store requests from ad blockers. _\*The "headers" are actually querystring values, so as not to trigger CORS issues, but the effect is the same_ Fixes getsentry/sentry-javascript#5406 * feat(angular): Add URL Parameterization of Transaction Names (#5416) Introduce URL Parameterization to our Angular SDK. With this change, the SDK will update transaction names coming from a URL with a paramterized version of the URL (e.g `GET /users/1235/details` will be parameterized to `GET /users/:userId/details/`). This is done by listening to the `ResolveEnd` routing event in `TraceService`. When this event is fired, the Angular router has finished resolving the new URL and found the correct route. Besides the url, the event contains a snapshot of the resolved and soon-to-be activated route. This `ActivatedRouteSnapshot` instance is the root instance of the activated route. If the resolved route is something other than `''` or `'/'`, it will also points to its first child. This instance might again point to its (a possibly existing) child but it also holds its part of the resolved and parameterized route path (URL). By recursively concatenating the paths, we get a fully parameterized URL. The main advantage of this approach vs. a previously tried URL<->parameter interpolation approach is that we do not run into the danger of making interpolation mistakes or leaving parts of the route unparameterized. We now simply take what the Angular router has already resolved. The potential disadvantage is that we rely on the assumption that there is only one child per route snapshot. While testing, I didn't come across a situation where a parent snapshot had more than one child. I believe that this is because the `ResolveEnd` event contains a snapshot of the newly activated route and not the complete router state. However, if we get reports of incorrect transaction names, we might want to revisit this parameterization approach. It should be noted that because there is a gap between transaction creation (where we initially set the unparameterized name) and URL parameterization, it is possible that parameterization might happen after an outgoing Http request is made. In that case, the dynamic sampling context will be populated and frozen without the `transaction` field because at this point the transaction name's source is still `url`. This means that we have a potential timing problem, similar to other routing instrumentations. At this point we're not yet sure how often such a timing problem would occur but it seems pretty unlikely for normal usage. For instance, DSC population will happen correctly (with the `transaction` field) when the first outgoing Http request is fired in the constructor of the component that is associated with the new route. This also means that hooks like `ngOnInit` which are frequently used for making requests (e.g. via Service calls) are called long after the `ResolveEnd` routing event. Additionally, this add a few unit tests to test this new behaviour. However, these tests are really unit tests, meaning they only test our `TraceService` implementation. We currently simply mock the Angular router and fire the routing events manually. A more "wholesome" approach (e.g. calling `router.navigate` instead of manually firing events) would be better but for this we'd need to inject an actual Angular Router into `TraceService`. This is done best with Angular's `TestBed` feature but it currently doesn't work with our testing setup for the Angular SDK. Changing the setup is out of scope for this PR but we'll revisit this and make the necessary changes to improve the testing setup of our Angular SDK. * meta: Update changelog for version 7.7.0 (#5421) * release: 7.7.0 * ref(nextjs): Remove compensation for workaround in `_error.js` (#5378) For a long time, the `_error.js` page we provide `@sentry/nextjs` users contained a workaround for vercel/next.js#8592. When most of the code in `_error.js` was moved into `captureUnderscoreErrorException`, parts of the workaround came along for the ride. Now that that issue has been fixed, the workaround is no longer necessary. This removes as many of the workaround's vestiges in `captureUnderscoreErrorException` as possible. (We can't remove them all, because the workaround existed in user code rather than ours, and we can't stop people from continuing to use it. This fixes things so that if they do that, it'll just bail gracefully.) * fix(remix): Clone erroneous responses not to consume their body streams. (#5429) Fixes: #5423 Ref: #5405 We were extracting the bodies of 4xx/5xx responses to capture, but Remix also uses them, we should not consume their `body` streams, as they are only available once in response lifespans. This PR updates `extractData` to work on a clone response. * fix(remix): Do not capture 4xx codes from thrown responses. (#5441) Fixes: #5425 Ref: #5429, #5405 As per review getsentry/sentry-javascript#5429 (comment), we need to define how and when we should capture a 4xx error. We will only capture thrown 5xx responses until then. * ref(angular): Set ErrorHandler Exception Mechanism to be unhandled by default(#3844) Previously, exceptions caught in the Sentry `ErrorHandler` defaulted to the generic mechanism, meaning they were marked as `handled: true` and `mechanism: generic`. This patch adds a custom mechanism for these events: `handled: false` (because they were caught in our ErrorHandler) and `mechanism: angular`. * ref(utils): Improve uuid generation (#5426) Since [modern browsers](https://caniuse.com/mdn-api_crypto_randomuuid) support `crypto.randomUUID()` we make use of it when generating UUIDs. This patch does the following: - Shaves ~160 bytes off the browser bundle - Modern platforms bail out quickly with `crypto.randomUUID()` - Less modern browsers (including IE11 and Safari < v15.4) use `crypto.getRandomValues()` - Node.js - `< v15` uses `Math.random()` - `v15 > v16.7 uses` `crypto.getRandomValues()` - `>= v16.7 uses` `crypto.randomUUID()` - Validated that all code paths do in fact return valid uuidv4 ids with hyphens removed! - Added tests to test all different kinds of random value and uuid creation * fix(nextjs): Stop using `eval` when checking for `sentry-cli` binary (#5447) In getsentry/sentry-javascript#4988, we switched to using `eval` in `ensureCLIBinaryExists` (called by our build-time config code in the nextjs SDK), in order to prevent Vercel's dependency-tracing algorithm from thinking the binary was a (n enormous) dependency and including it in people's serverless functions. (It was getting tricked by the `require.resolve()` call; turning that call into a string was the only way we could find to hide it from the algorithm.) But `eval` is kind of gross. And Rollup, which agrees it's gross, keeps yelling at us for using it. In order to suppress the warnings and generally clean things up, this replaces the `eval` with real code again, and in that real code replaces the `require.resolve()` call with a manual check of all of the paths `require.resolve()` would consider. No `require` means no confused algorithm means no erroneously bundled cli binary in Vercel. And no `eval` means happy Rollup means happy us, because now it's easier to see when the build has legit errors. Wins all around. * fix: no xhr transport * fix some type issues Co-authored-by: Jake Correa <jcorrea257@gmail.com> Co-authored-by: Lukas Stracke <lukas@stracke.co.at> Co-authored-by: Niko Felger <niko.felger@gmail.com> Co-authored-by: Luca Forstner <luca.forstner@sentry.io> Co-authored-by: getsentry-bot <bot@sentry.io> Co-authored-by: getsentry-bot <bot@getsentry.com> Co-authored-by: Onur Temizkan <onur@narval.co.uk> Co-authored-by: Abhijeet Prasad <aprasad@sentry.io> Co-authored-by: Anton Pirker <anton@ignaz.at> Co-authored-by: Katie Byers <lobsterkatie@gmail.com> Co-authored-by: Tim Fish <tim@timfish.uk>
This closed issue has been automatically locked because it had no new activity for a month. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you. |
Bug report
Describe the bug
Error.getInitialProps
is not called when an exception is thrown at the top level of a page while it is being loaded. This is problematic becausegetInitialProps
would have provided access to theError
object. I want the Error so that I can log it and potentially display it to the user in render().To Reproduce
I've created a minimal reproduction here: https://github.com/WestonThayer/bug-nextjs-error-getinitialprops
Expected behavior
The Error that was thrown should be made available to
_error.js
. I would expect to receive it viagetInitialProps
, but I supposed having it as a prop passed in torender
would work too.Additional context
I think this was introduced by #4764, specifically the lines that use the props from the server. I can see how that makes sense if the Error was thrown on the server side and
_error.js
'sgetInitialProps
ran on the server, but it doesn't account for code that works fine on the server, but throws on the client.As a workaround, the exception is passed as a prop to App, so I'm simply passing it on:
The text was updated successfully, but these errors were encountered: