kit icon indicating copy to clipboard operation
kit copied to clipboard

fix: 9475 adapter node log request error

Open dreitzner opened this issue 2 years ago • 2 comments

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • [x] It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • [x] This message body should clearly illustrate what problems it solves.
  • [x] Ideally, include a test that fails without this PR but passes with it.

Tests

  • [x] Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • [x] If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

Fixes #9475 This change will additionally log an error if the getRequest function in the SSR handler fails. I don't think we should test for an error log 😉 Will log an error similar to: image

dreitzner avatar Apr 18 '23 12:04 dreitzner

🦋 Changeset detected

Latest commit: dab875d8dcbb16e70ffd103c23b589b50e644e7c

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

This PR includes changesets to release 1 package
Name Type
@sveltejs/adapter-node 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

changeset-bot[bot] avatar Apr 18 '23 12:04 changeset-bot[bot]

I haven't had time to dig into this, just wanted to point out that you should add a test to assert that the warning is being logged. Here's how:

https://github.com/sveltejs/kit/blob/b48d58e77a8f807075f875f1f1aea420ccec3760/packages/kit/test/apps/basics/test/test.js#L830-L844

My question with this is whether it would get annoying and fill the logs. I'm not that familiar with how logging is configured in JavaScript. This category of issue doesn't indicate any issue with the application and so I'm not sure that error is the appropriate console level to use here.

benmccann avatar Aug 10 '23 20:08 benmccann

I've had a lot of trouble debugging an app running with the node adapter. There's literally nothing logged aside from the initial "listening" message. I think we need to go even further than this PR, but this is at least a good start to handle one kind of error condition. I've been googling for quite a while to better understand how to troubleshoot adapter-node apps and have come up empty-handed, and also tried asking on the Discord.

When you have a production application deployed with adapter-node, at some point you're going to want some logging for later troubleshooting. If you want to keep the noise down, we could use a log level or env var. I'm not quite sure what the typical JS idioms are here either, but I don't think complete silence is reasonable.

symbolicsorcerer avatar Aug 25 '23 04:08 symbolicsorcerer

This (and #9475) can now be closed — since #11289 (i.e. SvelteKit 2), creating the request will always succeed, and if there's an invalid body we'll find out when we try and read it:

export async function POST({ request }) {
  // if the request body is invalid, this will fail, and `handleError` will be invoked
  const body = await request.json();

  // ...
}

There is a small issue in that handleError is called with a status and message of 500/Internal Error rather than 413/Payload Too Large — this is because of a failed instanceof check resulting from some duplicated code — but the error itself contains the relevant details. Will open a separate issue for that.

Rich-Harris avatar Jan 10 '24 18:01 Rich-Harris