fix(dataplan-pg): type restrictions

[wesselvdv]

Description

fixes #1755

Performance impact

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.

[wesselvdv]

Applying this fix allows you to get actual type completion on a typed PgSelectStep<A> for singleRelation where the input of singleRelation is only valid if it exists as a relation in the associated PgRegistry.PgRelations for type A

[benjie]

Doesn't this reduce type safety by removing the constraints on the generics though?

[wesselvdv]

That's true, but the current constraints are too strict causing it to not be workable. I can't pass a object that can satisfy the constraints without causing it to be useless because I have to union "asd" with string keys causing it to be generic again.

[wesselvdv]

I also see that the use of undefined as bottom type is problematic in many cases, I think it's better to use never in such cases. And you don't need to explicitly indicate that it might be never in a union.

I am playing with it a bit, to see if I can get something working.

[benjie]

... but it's not "never" - it's literally that those things can be undefined. A function accepts an array of parameters (even an empty array), a table does not accept an array of parameters - the parameters are undefined - we should be able to differentiate this at a type level, otherwise we can't tell the difference between function-like and table-like... can we?

🤔 Thinking pause...

Maybe you're right... a table "never" accepts parameters... It's a bit weird because foo | never = foo for all foo... but I think we can still check it:

https://www.typescriptlang.org/play?#code/C4TwDgpgBAYlC8UB2BXAtgIwgJygH2QgDccBuAKAHpKpaA9AfnNEigHEFYoIAPYCJABMAzoRK4GUYNhTQAXFABmAQwA2wiBWq0ojZuGgAJTkmJkqNekxbQAkp2O9+Q0afFRJ02VAUr1mix09IA

My main concern is that this might look confusing to the consumer - they might think parameters is always required (since | never evaporates) and thus pass an empty array when they should be passing never. Currently the | undefined is very explicit that that's an option. Having said that, if it makes working with the types easier and doesn't actually break anything, I'm open to it.

[benjie]

Is it the [name in string] causing the issues for you; would [name: string] be any better? I'm not sufficiently knowledgeable about TypeScript to fully understand the difference.

[wesselvdv]

Is it the [name in string] causing the issues for you; would [name: string] be any better? I'm not sufficiently knowledgeable about TypeScript to fully understand the difference.

I am playing around with another one:

export interface PgRegistry<
  TCodecs extends Record<
    keyof TCodecs,
    PgCodec<any, PgCodecAttributes, any, any, any, any, any>
  >,
  TResourceOptions extends Record<
    keyof TResourceOptions,
    PgResourceOptions<
      string,
      TCodecs[keyof TCodecs],
      ReadonlyArray<PgResourceUnique<PgCodecAttributes>>,
      ReadonlyArray<PgResourceParameter>
    >
  >,
  TRelations extends Record<
    keyof TRelations,
    Record<
      keyof TRelations[keyof TRelations],
      PgCodecRelationConfig<
        any,
        PgCodec<string, PgCodecAttributes, any, any, never, any, never>,
        PgResourceOptions<any, PgCodecWithAttributes<any>, any, any>
      >
    >
  >,
> {
  pgCodecs: TCodecs
  pgResources: {
    [name in keyof TResourceOptions]: TResourceOptions[name] extends PgResourceOptions<
      infer UName,
      infer UCodec,
      infer UUniques,
      infer UParameters
    >
      ? PgResource<
          UName,
          UCodec,
          UUniques,
          UParameters,
          PgRegistry<TCodecs, TResourceOptions, TRelations>
        >
      : never
  }
  pgRelations: {
    [codecName in keyof TRelations]: {
      [relationName in keyof TRelations[codecName]]: Expand<
        Omit<TRelations[codecName][relationName], "remoteResourceOptions"> & {
          remoteResource: TRelations[codecName][relationName] extends {
            remoteResourceOptions: PgResourceOptions<
              infer UName,
              infer UCodec,
              infer UUniques,
              infer UParameters
            >
          }
            ? PgResource<
                UName,
                UCodec,
                UUniques,
                UParameters,
                PgRegistry<TCodecs, TResourceOptions, TRelations>
              >
            : never
        }
      >
    }
  }
}

[benjie]

Foo extends Record<keyof Foo, ...>

But the... wait the key... the... of... of the... ... what?! 🤯

[wesselvdv]

I've tried some approaches closely aligned with how it's done now, and the performance goes down the drain quite spectacularly. I am now looking at using Tuple like constructs as type parameters vs record like ones, and it seems that this has a positive effect on the performance and inference. Still playing around with it though, as I am still resolving inference issues as I go.

[benjie]

I appreciate your work on this @wesselvdv; I'm going to convert this PR to a "draft" to indicate that research is still in progress. You can probably see I tried a bunch of approaches and the current one is the best I could come up with, but I still very much do not like it, and it requires too much manual as ... casting - not to mention not accepting perfectly valid things that it should accept.

[wesselvdv]

I appreciate your work on this @wesselvdv; I'm going to convert this PR to a "draft" to indicate that research is still in progress. You can probably see I tried a bunch of approaches and the current one is the best I could come up with, but I still very much do not like it, and it requires too much manual as ... casting - not to mention not accepting perfectly valid things that it should accept.

I closed this one in favour for my other branch, for which I have created a new PR.#1784