🗺 Fix server error logging and add onUnhandledError support #6495

brophdawg11 · 2023-05-26T19:43:04Z

This PR does 2 things (broken up by commit for easier reviewing):

Ensure we're logging unsanitized errors to the server console in produciton - eventhough we only send the sanitized errors to the browser
- This fixes a regression from the last set of fixes to ensure all code paths would sanitize before hydrating
While in this code, I also added support for a handleError export from entry.server (see Server-Side Uncaught Error Handler #2190).
- The default implementation of handleError if not provided is our previous logServerErrorIfNotAborted which will log in non-test environments and only if the request wasn't aborted
- This is the only logging we do from the server, so if you provide your own handleError then Remix will do no logging (except from the express middleware built into remix-serve) and it's entirely up to you to log from your handleError method.

TODO:

Add Docs

changeset-bot · 2023-05-26T19:43:07Z

🦋 Changeset detected

Latest commit: acd8f0e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 18 packages

Name	Type
@remix-run/server-runtime	Minor
@remix-run/cloudflare	Minor
@remix-run/deno	Minor
@remix-run/dev	Minor
@remix-run/node	Minor
@remix-run/react	Minor
@remix-run/cloudflare-pages	Minor
@remix-run/cloudflare-workers	Minor
create-remix	Minor
@remix-run/css-bundle	Minor
@remix-run/architect	Minor
@remix-run/express	Minor
@remix-run/netlify	Minor
@remix-run/serve	Minor
@remix-run/testing	Minor
@remix-run/vercel	Minor
remix	Minor
@remix-run/eslint-config	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

integration/error-sanitization-test.ts

brophdawg11 · 2023-05-26T19:47:30Z

integration/headers-test.ts

@@ -12,193 +13,196 @@ test.describe("headers export", () => {
  let appFixture: Fixture;

  test.beforeAll(async () => {


Ignore changes in here, I added ServerMode.Test to suppress some thrown response logging, and the rest is whitespace changes for the updated indentation

brophdawg11 · 2023-05-26T19:49:10Z

packages/remix-react/errorBoundaries.tsx

+            >
+              {error.stack}
+            </pre>
+          ) : null}


No need to include an empty <pre> with padding if we have no stack right? It's just an empty red box on the screen

packages/remix-server-runtime/build.ts

packages/remix-server-runtime/server.ts

brophdawg11 · 2023-05-30T14:43:04Z

packages/remix-server-runtime/__tests__/server-test.ts

-      expect(calls.length).toBe(2 * DATA_CALL_MULTIPIER);
-      expect(spy.console.mock.calls.length).toBe(1 * DATA_CALL_MULTIPIER);
+      expect(calls.length).toBe(2);
+      expect(spy.console.mock.calls[0][0].data).toEqual(
+        'Error: No route matches URL "/"'
+      );
+      expect(spy.console.mock.calls[1][0].message).toEqual(
+        "thrown from handleDocumentRequest and expected to be logged in console only once"
+      );
+      expect(spy.console.mock.calls[2][0].message).toEqual(
+        "second error thrown from handleDocumentRequest"
+      );
+      expect(spy.console.mock.calls.length).toBe(3);


This tests gets updated now that we're properly rendering these from the server request handle and not from the render cycle

brophdawg11 · 2023-06-01T14:31:40Z

packages/remix-dev/config/defaults/cloudflare/entry.server.react-stream.tsx

+        // Log streaming rendering errors from inside the shell
        console.error(error);
        responseStatusCode = 500;


This is only async rendering errors, shell rendering errors will cause the promise to reject.

brophdawg11 · 2023-06-01T14:34:14Z

packages/remix-dev/config/defaults/node/entry.server.react-stream.tsx

+          // Log streaming rendering errors from inside the shell.  Don't log
+          // errors encountered during initial shell rendering since they'll
+          // reject and get logged in handleDocumentRequest.
+          if (shellRendered) {
+            console.error(error);
+          }


Only log async rendering errors. Shell-rendering errors will also call this function and then will reject the promise via onShellError and we'll handle logging those in the server (or sending them to onUnhandledError if available)

brophdawg11 · 2023-06-01T14:35:43Z

packages/remix-server-runtime/server.ts

-      logServerErrorIfNotAborted(error.error || error, request, serverMode);
+      if (error.error) {
+        handleError(error.error);
+      }


At this point these should always have an error prop since they should all be internal-router-generated errors. User-thrown Responses would have been returned as a plain Response above. But just to be safe, we don't want user-thrown Responses logging

brophdawg11 · 2023-06-01T14:36:33Z

packages/remix-server-runtime/server.ts

+    Object.values(context.errors).forEach((err) => {
+      if (!isRouteErrorResponse(err) || err.error) {
+        handleError(err);
+      }
+    });


Log non-ErrorResponse's as well as internal router-generated ErrorResponse's

brophdawg11 · 2023-06-01T14:37:58Z

integration/error-sanitization-test.ts

+              export function onUnhandledError(
+                error: Error | unknown,
+                { request }: { request: Request },
+              ) {
+                console.error("App Specific Error Logging:");
+                console.error("  Request: " + request.method + " " + request.url);
+                if (error instanceof Error) {
+                  console.error("  Error: " + error.message);
+                  console.error("  Stack: " + error.stack);
+                } else {
+                  console.error("Dunno what this is");
+                }
+              }


Custom user-provided logging for tests

brophdawg11 · 2023-06-01T14:39:02Z

packages/remix-server-runtime/server.ts

+  let errorHandler =
+    build.entry.module.onUnhandledError ||
+    ((error, { request }) => {
+      if (serverMode !== ServerMode.Test && !request.signal.aborted) {
+        console.error(error);
+      }
+    });


User-logging and built-in-logging are mutually exclusive

packages/remix-react/errorBoundaries.tsx

.changeset/on-unhandled-error.md

packages/remix-dev/config/defaults/node/entry.server.react-stream.tsx

quinn · 2023-06-01T22:43:39Z

Now that this is merged, what remix version will this be included in?

github-actions · 2023-06-02T07:10:02Z

🤖 Hello there,

We just published version v0.0.0-nightly-70cc627-20230602 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

github-actions · 2023-06-02T18:31:43Z

🤖 Hello there,

We just published version 1.17.0-pre.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

github-actions · 2023-06-06T19:30:28Z

🤖 Hello there,

We just published version 1.17.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

fix: ensure server errors are logged before sanitizing

fc512e5

remix-cla-bot bot added the CLA Signed label May 26, 2023

brophdawg11 force-pushed the brophdawg11/server-errors branch from 671ffcf to 469253c Compare May 26, 2023 19:44

brophdawg11 commented May 26, 2023

View reviewed changes

integration/error-sanitization-test.ts Outdated Show resolved Hide resolved

brophdawg11 force-pushed the brophdawg11/server-errors branch from 469253c to b5dc636 Compare May 26, 2023 19:46

brophdawg11 commented May 26, 2023

View reviewed changes

packages/remix-server-runtime/build.ts Outdated Show resolved Hide resolved

feat: support handleError export from entry.server

138db33

brophdawg11 force-pushed the brophdawg11/server-errors branch from b5dc636 to 138db33 Compare May 26, 2023 19:49

brophdawg11 commented May 26, 2023

View reviewed changes

packages/remix-server-runtime/server.ts Outdated Show resolved Hide resolved

brophdawg11 commented May 26, 2023

View reviewed changes

packages/remix-server-runtime/server.ts Outdated Show resolved Hide resolved

brophdawg11 self-assigned this May 26, 2023

brophdawg11 added the package:server-runtime label May 26, 2023

Fix up server unit test

7a92761

blittle mentioned this pull request May 26, 2023

PoC: Add a logging abstraction and wrap remix build output to track errors Shopify/hydrogen#941

Closed

brophdawg11 commented May 30, 2023

View reviewed changes

brophdawg11 added 4 commits May 30, 2023 15:25

Update changelog with example

9617e7f

Merge branch 'dev' into brophdawg11/server-errors

b41a33c

Rename from handleError -> onUnhandledError

049bd04

Export OnUnhandledErrorFunction type

403b5d9

brophdawg11 changed the title ~~Fix server error logging and add handleError support~~ Fix server error logging and add onUnhandledError support May 31, 2023

brophdawg11 commented Jun 1, 2023

View reviewed changes

brophdawg11 force-pushed the brophdawg11/server-errors branch from 3773f4e to c9f9e2e Compare June 1, 2023 14:32

Add logging back to entry-server for streamed html chunks

b87dadf

brophdawg11 force-pushed the brophdawg11/server-errors branch from c9f9e2e to b87dadf Compare June 1, 2023 14:33

brophdawg11 commented Jun 1, 2023

View reviewed changes

packages/remix-react/errorBoundaries.tsx Show resolved Hide resolved

brophdawg11 commented Jun 1, 2023

View reviewed changes

.changeset/on-unhandled-error.md Outdated Show resolved Hide resolved

brophdawg11 commented Jun 1, 2023

View reviewed changes

packages/remix-dev/config/defaults/node/entry.server.react-stream.tsx Show resolved Hide resolved

brophdawg11 added 4 commits June 1, 2023 16:37

Add console.error back to client side error boundary

0cf1ae0

Remove uneccesary Error | unknown typing

2891fbc

Update templates

c5d8728

Fix unintended logging in unit test

acd8f0e

brophdawg11 merged commit c6d8c37 into dev Jun 1, 2023

brophdawg11 deleted the brophdawg11/server-errors branch June 1, 2023 22:12

This was referenced Jun 2, 2023

chore: Update version for release (pre) #6525

Closed

chore: Update version for release (pre) #6526

Merged

github-actions bot mentioned this pull request Jun 6, 2023

chore: Update version for release #6550

Merged

hi-ogawa mentioned this pull request Aug 11, 2023

feat: "on error" callback to intercept loader error? hi-ogawa/vite-plugins#84

Closed

MichaelDeBoey changed the title ~~Fix server error logging and add onUnhandledError support~~ 🗺 Fix server error logging and add onUnhandledError support Jan 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🗺 Fix server error logging and add onUnhandledError support #6495

🗺 Fix server error logging and add onUnhandledError support #6495

brophdawg11 commented May 26, 2023 •

edited

Loading

changeset-bot bot commented May 26, 2023 •

edited

Loading

brophdawg11 May 26, 2023

brophdawg11 May 26, 2023

brophdawg11 May 30, 2023

brophdawg11 Jun 1, 2023

brophdawg11 Jun 1, 2023

brophdawg11 Jun 1, 2023

brophdawg11 Jun 1, 2023

brophdawg11 Jun 1, 2023

brophdawg11 Jun 1, 2023

quinn commented Jun 1, 2023

github-actions bot commented Jun 2, 2023

github-actions bot commented Jun 2, 2023

github-actions bot commented Jun 6, 2023

		@@ -12,193 +13,196 @@ test.describe("headers export", () => {
		let appFixture: Fixture;

		test.beforeAll(async () => {

🗺 Fix server error logging and add onUnhandledError support #6495

🗺 Fix server error logging and add onUnhandledError support #6495

Conversation

brophdawg11 commented May 26, 2023 • edited Loading

changeset-bot bot commented May 26, 2023 • edited Loading

🦋 Changeset detected

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quinn commented Jun 1, 2023

github-actions bot commented Jun 2, 2023

github-actions bot commented Jun 2, 2023

github-actions bot commented Jun 6, 2023

brophdawg11 commented May 26, 2023 •

edited

Loading

changeset-bot bot commented May 26, 2023 •

edited

Loading