Unify API/Enhance Type-Safety

[n1ru4l]

Closes https://github.com/sikanhe/gqtx/pull/49 Closes https://github.com/sikanhe/gqtx/issues/48 Closes https://github.com/sikanhe/gqtx/issues/14 Closes https://github.com/sikanhe/gqtx/issues/37

Hey, @sikanhe I took over #49 and applied some further improvements to the API (optional resolve if the source type has a property that matches the field type) as well as adding some tests for the typescript API and other smaller improvements

[sikanhe]

Interesting, it looks like we need to make bigger changes to make field function to work for both resolver vs no-resolver scenarios?

• What is the actual problem we are solving with this? Is having a unified API important? Is this actually better than having an explicit function for each use case? This makes the types more complicated while not gaining much.
• What about abstractField ?
• There are people rely on current API in production - making a big API change like this, unless it is a clear winner, is not preferred.

There is also a lot of formatting/style change to parse the before/after difference.

Lmk your thoughts!

[n1ru4l]

The main problem is actually that the defaultField function right now is not really type-safe. Having both a defaultField and field api does not allow this as afaik, as we cannot cause/raise a typescript error if the defaultField type does not match the Source[fieldName] type.

I missed abstractField, need to add this as well.

afaik gqtx is not adopted that much and the api currently has some flaws. Making the breaking changes now ensures we dont have to do them when the library receives broader adoption.

[n1ru4l]

@sikanhe I further adjusted the abstractField API and also made the subscriptionField API type-safe. Furthermore, I added a TypeScript type test and a jest runtime test to ensure the functionality works as expected.

What is the actual problem we are solving with this? Is having a unified API important? Is this actually better than having an explicit function for each use case? This makes the types more complicated while not gaining much.

I thought about this a bit more and to me, it seems that we cannot provide a full type-safe API for field without complicating the types.

I also created https://github.com/dungeon-revealer/dungeon-revealer/pull/1389/files to showcase the upgrade path on a more complex project. I adjusted all the fields using regex search and replace within vscode, then manually adjusted the subscription field resolvers.

[n1ru4l]

Hey @sikanhe, could you have a second look at this?

[sikanhe]

@n1ru4l Hey I have been a little bit overwhelmed with my job - I put sometime on my calendar to take a look this this weekend.

[sikanhe]

@n1ru4l Hey the PR looks fine - do you think we can move the prettier changes to another PR? It is hard to parse out which lines are the real change. Then i will take a one last look

[n1ru4l]

@sikanhe yes sure, will ping you once done 😇

[n1ru4l]

@sikanhe I reverted all the quote changes that introduced a lot of noise. I can still do more if you want, but I think that should be enough

[n1ru4l]

@sikanhe Are you fine with me merging this as you approved this?

[sikanhe]

@n1ru4l Yes!

[andrew0]

Thanks a lot for your effort on this feature - these API changes definitely seem to be better. I'm trying out the canary for this, and I'm running into some strange errors, though. I get errors when I try to use an interface for the source object, but it works fine if I use a type.

For example, this works fine:

import { createTypesFactory } from 'gqtx';

const t = createTypesFactory();

type TestObject = {
  foo: string;
}

const TestObjectType = t.objectType<TestObject>({
  name: 'foo',
  fields: () => [
    t.field({name: 'foo', type: t.NonNull(t.String)}),
  ],
});

If I use an interface, though, I get an error:

import { createTypesFactory } from 'gqtx';

const t = createTypesFactory();

interface TestObject {
  foo: string;
}

const TestObjectType = t.objectType<TestObject>({
  name: 'foo',
  fields: () => [
    t.field({name: 'foo', type: t.NonNull(t.String)}), // type error on this line
  ],
});

Type 'Field<undefined, Record<string | number | symbol, unknown>, any, any>' is not assignable to type 'Field<any, TestObject, any, {}>'.
  Types of property 'resolve' are incompatible.
    Type '(src: Record<string | number | symbol, unknown>, args: TOfArgMap<ArgMap<any>>, ctx: undefined, info: GraphQLResolveInfo) => any' is not assignable to type '(src: TestObject, args: TOfArgMap<ArgMap<{}>>, ctx: any, info: GraphQLResolveInfo) => any'.
      Types of parameters 'src' and 'src' are incompatible.
        Type 'TestObject' is not assignable to type 'Record<string | number | symbol, unknown>'.ts(2322)
Argument of type '{ name: "foo"; type: OutputType<undefined, string>; }' is not assignable to parameter of type '{ name: "foo"; type: OutputType<undefined, string>; args?: ArgMap<unknown> | undefined; description?: string | undefined; deprecationReason?: string | undefined; extensions?: unknown; } & ResolvePartialMandatory<...>'.
  Property 'resolve' is missing in type '{ name: "foo"; type: OutputType<undefined, string>; }' but required in type 'ResolvePartialMandatory<Record<string | number | symbol, unknown>, unknown, undefined, string>'.ts(2345)

Is this expected?

edit: upon further investigation, it seems like when I used the ^0.8.1-348cf7b3.0 range, it was resolving to 0.8.1-f565abfa.0. The issue was that Src was constrained to Src extends Record<string | number | symbol, unknown> which doesn't work with interfaces. When I resolved to 0.8.1-348cf7b3.0 exactly, it doesn't have this constraint and the issue went away.

[n1ru4l]

@andrew0 Doed that mean everything is good? We can also add this to the API test cases to ensure it wont break in the future.

[andrew0]

@n1ru4l yes, everything is good. The issue was fixed by this commit, where field<TKey extends string, Src extends Record<string | number | symbol, unknown>, Arg, Out> was changed to field<TKey extends string, Src, Arg, Out>. I only ran into the issue because I had accidentally installed an older version.