Rework TypeScript types around registry, codecs, resources and relations

[wesselvdv]

Description

In this PR i've tried to rework to typescript system so it would be more ergonomic for both internal and external usage. Please be aware that it's mostly a showcase of how it could be, not how it should be.

Observations leading to the changes in this PR:

• Missing inference in some place due to incorrect type constraints or the usage of indexed access which causes early evaluation to any.
• Impossible to adhere to the PgRegistry interface because of the type parameter being objects ({ [indexName: string]: etc }) which causes early evaluation to string which loses the actual properties.
• Usage of keyof when inferring objects, which can cause early evaluation resulting in string | number even when explicitly defining an object with index string: keyof { [index: string]: number } === string | number
• Too much type casting to 'force' the correct type signature
• Too broad, or too specific constraints forcing the above.
• Using undefined as bottom type instead of never

Changes in this PR:

• Removal of most default type parameters and addition of explicit Any<PgType> and Default<PgType> interfaces.
  • The Any<PgType> interfaces are for internal usage, and the Default<PgType> for external usage. This because the latter ones cause massive circular reference issue for typescript when used in places where they're being defined. (e.g. inside dataplan/pg, but not in graphile-build-pg)
  • The default interfaces have the entire default structure in place, whereas the any interfaces only have the base structure for example:

  DefaultPgResource['codec'] === DefaultPgCodec
  AnyPgResource['codec'] === any

• Replacement of object and array like type parameters with unions
  • This also reduces the need for Expand<> which is quite performance intensive.
  • Also allows for easier manipulation of type parameters and less performance impact (mainly versus intersections), for example for PgRegistry:

//old

addResource<const TResource extends PgResourceOptions<any, any, any, any>>(
    resource: TResource,
  ): PgRegistryBuilder<
    TCodecs & {
     [name in TResource["codec"]["name"]]: TResource["codec"];
   },
   TResources & {
     [name in TResource["name"]]: TResource;
   },
  TRelations
>;

//new
addResource<const TResourceOption extends AnyPgResourceOptions>(
    resource: TResourceOption,
): PgRegistryBuilder<
    TCodecs | PgResourceOptionCodec<TResourceOption>,
    TResourceOptions | TResourceOption,
    TRelationConfigs
  >;

• Removal of indexed access and more infer to delay the inference internally and have less type casting.

The changes in dataplan/pg are the core changes, and the resulting changes in graphile-build-pg and others are a consequence of this. The changes in the latter have mostly been changing PgCodec to DefaultPgCodec and removing a LOT of explicit type casts because this now "just" works.

Biggest achievement which I am not sure if this is because of this PR, but nonetheless useful. The correct typing of the below snippet in the exampleSchema:

crystal/grafast/dataplan-pg/src/examples/exampleSchema.ts

Lines 3386 to 3396 in 5589d14

	const RelationalTopic = newObjectTypeBuilder<
	OurGraphQLContext,
	RelationalTopicStep
	>(PgSelectSingleStep)({
	name: "RelationalTopic",
	interfaces: [RelationalItem],
	fields: () => ({
	...commonRelationalItemFields(),
	title: attrField("title", GraphQLString),
	}),
	});

Having the ability to define a correctly typed parent step with a specific resource, and having it infer correctly and type check that it adheres to said parent. (e.g. RelationalTopicStep extends RelationalItemsLikeStep ? 'yes' : 'no' === 'yes')

Outstanding tasks

• Groom the codebase for more consistency in type names, and type helper names. (e.g. GetPgCodecAttributes vs PgCodecAttributes, or even consider namespaces PgCodec.getAttributes<> | PgCodec.Attributes<>)
• Remove all default type parameters
• Investigate more type inference capabilities. (unions/polymorphism)

fixes #1755

Performance impact

Non empirical observation is that the exampleSchema.ts file has become significantly faster.

Security impact

Checklist

• My code matches the project's code style and yarn lint:fix passes.
• I've added tests for the new feature, and yarn test passes.
• I have detailed the new feature in the relevant documentation.
• I have added this feature to 'Pending' in the RELEASE_NOTES.md file (if one exists).
• If this is a breaking change I've explained why.

[changeset-bot]

⚠️ No Changeset found

Latest commit: b364568

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

[wesselvdv]

@benjie I think I found a way to delay the inference without causing any to fly around the code constantly:

export interface AnyPgCodec extends PgCodec {} // <-- allows use of default generic parameters, and prevents "circular default detected" errors 
/**
 * A codec for a Postgres type, tells us how to convert to-and-from Postgres
 * (including changes to the SQL statement itself). Also includes metadata
 * about the type, for example any of the attributes it has.
 */
export interface PgCodec<
  TName extends string = string,
  TCodecAttributes extends
    ReadonlyArray<PgCodecAttribute> = ReadonlyArray<PgCodecAttribute>,
  TFromPostgres = any,
  TFromJavaScript = TFromPostgres,
  TArrayItemCodec extends AnyPgCodec = AnyPgCodec, // <-- using just PgCodec here causes circular default parameter issues
  TDomainItemCodec extends AnyPgCodec = AnyPgCodec,
  TRangeItemCodec extends PgRangeItemCodec = PgRangeItemCodec,
> {
// snip
}

For example, I think I might've been able to get PgResource.resolveVia to type check without casting using the above method. Also seems that the use of Expand<> is no longer required.

I haven't done all of it yet, but it seems promising.

[benjie]

Exciting! This reminds me of the kind of trick you need to do to build a JSONValue type, I should have thought about that - the difference between eager evaluated and lazily evaluated types... I'm not yet good enough at TypeScript to figure these things out for myself. Keep up the great work!

[wesselvdv]

Exciting! This reminds me of the kind of trick you need to do to build a JSONValue type, I should have thought about that - the difference between eager evaluated and lazily evaluated types... I'm not yet good enough at TypeScript to figure these things out for myself. Keep up the great work!

Thanks, I've got a basic working example schema again. There are still loads of mismatches, and biggest one is the PgUnionAllStep atm. I think you chose wisely to not try and type the associated pgresources there, but I am gonna try.

[benjie]

The ! shouldn't be required here since pgCodecAttribute will only ever be called with a PgCodec that has attributes (formerly a PgCodecWithAttributes)

[benjie]

Should this (and the related changes below) have been DefaultPgCodecWithAttributes or similar?

[wesselvdv]

Yes, but this gave me a lot of errors, so I opted to widen the constraint for now, as I felt it wasn't necessarily a requirement for the purpose of this PR.

[benjie]

I think you've removed the newline from the end of .gitignore - perhaps when you removed the nix stuff?

[wesselvdv]

I'll put it back, :-)

[benjie]

I've merged the latest and I've renamed AnyPg* to _AnyPg* to make it clear it's internal, and I've renamed DefaultPg* to GenericPg* to show that e.g. GenericPgCodec represents a generic (arbitrary) PgCodec.

DefaultPgCodec I found quite confusing until I realised it meant PgCodecWithDefaultGenericParameters; I had interpretted it a different way. I discussed a number of possible options with @jemgillam and in the end GenericPgCodec won out, even though ironically it doesn't have any generics!

[benjie]

If I change via and identicalVia to GenericPgCodecAttributeVia then I get weird errors in exampleSchema.ts. However, I know that if via is set then it will always be string | { relation: string, attribute: string } (or a narrower form of this) so I don't really understand why this needs to use anys?

[benjie]

I've also fixed the linting issues, but I'm a bit lost in understanding what we're actually achieving here - seems that we're hitting any pretty fast in internal code, which gives me pause.

[benjie]

(To be clear: the previous comment was stating that I'm lost because of my poor understanding of TypeScript, not because the PR is bad. It could be that we were hitting any pretty soon already, and I just didn't realise it - I find it quite hard confirming TypeScript types sometimes, especially when the hover hints are so huge. Or it could be that the any's internally are desired in order to reduce the any's that consumers receive; in which case that's a trade-off I'm willing to make but would require additional testing (or casting). I just started at the top of the PR and got lost pretty quickly (see comment) and am looking for some guidance/understanding.)

[benjie]

(@wesselvdv and I had a long call where we went through this, and unfortunately though it solves some issues it also creates others, in particular making the codebase harder to work on. Currently the _Any* and Generic* types don't adhere to the rules that we'd like (library consumers use Generic* and internally we use _Any*), and we can get into some tricky situations; we're hoping that we can figure out some better rules and apply them to a clean checkout of the codebase. Leaving this open for context and as a reminder.)