Fix/ts resolvers type

[dac09]

Closes #5370

What does this PR do?

1. Adds the makeResolverTypeCallable: true to graphql-codgen config
2. Makes the obj optional, because we don't use it in testing.

This is a little dubious, because obj is always included when actually using the resolving, but it just feels like a lot of extra code when testing services to pass an empty object.
3. Adds g sdl to CI, and moves typecheck to the end so we check all the generated code (so this sort of regression doesn't happen again)

---

Just a little headsup on this fix - it has a downside... explained here: https://s.tape.sh/nK9CgxZt

Please read comments in the PR

[netlify]

✅ Deploy Preview for redwoodjs-docs canceled.

Name	Link
🔨 Latest commit	68b781a
🔍 Latest deploy log	https://app.netlify.com/sites/redwoodjs-docs/deploys/62793df7bc7a070008039237

[dac09]

Just a little headsup on this fix - it has a downside... explained here: https://s.tape.sh/nK9CgxZt

I'm trying a couple of other things to see if I can find another way

[orta]

Depending on how thorough you want to be here, you could switch strict to be true in the tsconfig to really ensure things are valid for all consumers

[dac09]

Hmmm good point! I don’t think I’ve ever tried it in strict mode 😂

[dac09]

@orta I was thinking of another approach - wanted your thoughts on it!

If we made ResolverFn an interface (probably would need to change stuff in codegen) - then, Only for jest, override this interface two have two forms:

1. One that takes no arguments
2. Another that takes the usual args and obj.

What do you think?

[orta]

After watching the video, I'm not super sold on making both params optional. It's gonna require either using ! or checking in your resolvers for those objects which are always there in prod - but maybe not in tests. Making writing resolvers in strict mode more inaccurate.

The reason it feels inelegant is because the redwood generators recommend using argument destructuring to hide the actual object, if it was just an arg object.

args should be added to the test generators because that matches the graphql resolver API imo, I think the 2nd obj param could be optional but it's weird because that's not true in reality either

[dac09]

Yeah totally agree - I feel icky from trying it

[orta]

Yeah, interesting approach, I'm not too certain how that would work out form the top of my head. Something like:

    customResolverFn: `(
      args: TArgs,
      obj: { root: TParent; context: TContext; info: GraphQLResolveInfo }
    ) => Promise<Partial<TResult>> | Partial<TResult>;`,

to

    customResolverFn: `() => Promise<Partial<TResult>> | Partial<TResult>;
  (
      args: TArgs,
      obj: { root: TParent; context: TContext; info: GraphQLResolveInfo }
    ) => Promise<Partial<TResult>> | Partial<TResult>;`,

Which should probably set up two callable functions overloads externally when using resolver inside the type, then because you have passed some arguments in during implementation TS would narrow to the right one in resolver implementation, and you probably get the choice from outside the module to just pass nothing in

[dac09]

Ah that’s a good idea - not sure why I was thinking of an interface! I’ll give it a try in an hour or so!

[dac09]

Ah just remembered. Interface because I only want to override the type for jest tests, and not the actual source. Getting codegen to generate an interface on the other hand could be a challenge!

[dac09]

Which should probably set up two callable functions overloads externally when using resolver inside the type, then because you have passed some arguments in during implementation TS would narrow to the right one

Here's what I tried:

type OGResolver<TResult, TParent, TContext, TArgs> = (
  args: TArgs,
  obj?: { root: TParent; context: TContext; info: GraphQLResolveInfo }
) => Promise<Partial<TResult>> | Partial<TResult>

type ResolverFnForTests<TResult> = () =>
  | Promise<Partial<TResult>>
  | Partial<TResult>

export type ResolverFn<TResult, TParent, TContext, TArgs> =
  | ResolverFnForTests<TResult>
  | OGResolver<TResult, TParent, TContext, TArgs>

Unfortunately looks like TS doesn't narrow correctly ☹️🌴 (face palm) in the test

---

I tried the interface approach too, like this:

// eslint-disable-next-line @typescript-eslint/no-unused-vars
interface ResolverFn<TResult, TParent, TContext, TArgs> {
  (): Promise<Partial<TResult>> | Partial<TResult>
}

interface ResolverFn<TResult, TParent, TContext, TArgs> {
  (
    args: TArgs,
    obj?: { root: TParent; context: TContext; info: GraphQLResolveInfo }
  ): Promise<Partial<TResult>> | Partial<TResult>
}

This one has the inverse problem, where I think its also picking the wrong interface (the ts error is gibberish) but instead of in the test, in the actual source. I also realised I can't just override types for just Jest... must've gotten confused.

[dac09]

Ok - so I've looked into strict mode more - and here's my take on this.

a) Strict mode just throws errors all over the place - including from Prisma, auth, directives, etc.
I think we'll need a concerted effort to really support strict mode out of the box. If nothing else maybe we need to document enabling strict mode and all the little things that a user would need to change/configure to use it.

I know this probably sounds like a cop out to you Orta, but my gut says that people using strict mode know what they're buying into, and would be ok with dealing with the additional headache of maybe overriding the resolver type, and adding empty objects in tests. i.e. they opt-in to more complexity

b) I still think making the resolver function params optional (this PR's implementation) is the lesser of two evils, as icky as it is. It resolves the issue, and without strict mode, you still have access to args and info, root, etc. with no changes to your code in either source or test.

c) Even if the rest of the team agreed to add {} to the service generators, this would count as a breaking change for existing projects, so a no-go for me.

I'll sleep on this, would love some more opinions!

[orta]

Aye, it's only the lesser problem from the perspective of writing a test - but but when making them optional, you make using strict mode very hard to use. In my app, switching to this breaks every resolver because you have to prove the existence of those params in the real code.

e.g. can't use destructuring

And you'd you have to put arg! or obj! somewhere to undo it etc. Might need to be some way to customize this then.

[dac09]

Yes I'm thinking (in a separate PR) I'll add some docs and a config flag to generate a different ResolverFn for strict mode.

For now though you could override the resolver type by adding a codegen.yaml to the root of your project

[jtoar]

Spent some time trying to get that function overload working as well; came back empty handed, but still holding out hope we can resolve something sometime soon, maybe in collaboration with the guild. After this, we'll make a concerted effort towards strict mode to make sure it's not an afterthought but a legitimate redwood workflow.

Right now we see this as a fix for type-check. But we'll continue discussing it more tomorrow at the meeting.

[dac09]

To be continued in #5481