feat(graphql): Improve GraphQL Yoga fatal exception handling

[will-ks]

Currently the GraphQL Yoga initialization code is wrapped in two separate try catch blocks.

In the second block, exceptions are not currently logged, and if an exception is caught here, the GraphQL server fatally crashes but you get no useful logging- you just get this:

[0] api | /Users/willks/projects/redwood-saas-starter/node_modules/pino/lib/tools.js:59
[0] api |       if (typeof this[msgPrefixSym] === 'string' && msg !== undefined && msg !== null) {
[0] api |                      ^
[0] api | 
[0] api | TypeError: Cannot read properties of undefined (reading 'Symbol(pino.msgPrefix)')
[0] api |     at LOG (/Users/willks/projects/redwood-saas-starter/node_modules/pino/lib/tools.js:59:22)
[0] api |     at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
[0] api | 

It doesn't seem to be useful to separate the initialization code into two try catches and treat them differently, as exceptions in either block are fatal, so this PR combines the two blocks. Now you get proper pino logging for exceptions wherever they occur.

Also, when I was initially trying to debug this, before looking into redwood's code, I thought about using the onException callback option in GraphQLYogaOptions, but it's not all that useful as it doesn't pass through the original exception. So this PR also changes that to pass through the original exception.

[Tobbe]

Thanks for the PR @will-ks! I'll ask @dthyresson to take a look when he has a few minutes to spare 🙂

[dthyresson]

Ah, that's some very original createYoga code I think :) @will-ks and I'll have to see if there was a reason we did that ... though it may have been an oversight and if so, this PR is great to correct that! I'll review and let know.

[dthyresson]

@will-ks The reason we have two try catch blocks is the they serve to handle two separate cases:

The first block, assembling schema and validating it ... this provides a similar check that happens during codegen. If the schema is invalid, then we dont' want the GraphQL server to start up at all; it's a preemptive error and such the process exits.

The second block is intended to handle runtime or exceptions that happen when the server is created. This will not kill the process like the first does and shouldn't.

That said, I do agree that both blocks should log the error.

The first is a fatal level -- the second should be an error level.

If we were to change the onException() to pass an error:

  /**
   * @description A callback when an unhandled exception occurs. Use this to disconnect your prisma instance.
   */
  onException?: () => void

that might be a breaking change. It was intended to handle the exception, not log it.

So - how about we just add a console.error() in

  } catch (e) {
   console.error(e.message, e)
    if (onException) {
      onException()
    }
    throw e
  }

I think the will provide the information to help fix an y errors that pop up but also doesn't kill the server on a runtime error -- and maintains the behavior of a fatal error if the schema is invalid.

Also, one of the reasons we won't the diet block to kill the server is this case:

api | [GQL Server Error] - Schema validation failed
api | You must specify one of @requireAuth, @skipAuth or a custom directive for
api | - logEnvironment Query 
api | 
api | ----------------------------------------
web | 11:39:42 AM [vite] http proxy error: /graphql

Here we did schema validation and there is no directive. So, we'll never start the server.

This ensures that GraphQL operations are covered by some auth/permission directives.

[will-ks]

Hi @dthyresson
Sure, I don't have the full picture of the process lifecycle, so definitely if it's more appropriate to just log there I can change the PR to just do that.

So - how about we just add a console.error() in

Would it not be better to use logger? I've force pushed the branch to use that.

[dthyresson]

Hey @will-ks sorry we have been busy on some other features and I apologies for the late reply. I know seems a bit silly maybe to iterate on such a small Pr but is there a way I can reproduce this in a new project? I am tying to think of a way or place that the exception would be logged like you had in your description.

Looks like something with Pino logger?

I just want to reproduce the case to see what it looks like and then merge.

Thanx for understanding.