remove filter never from select and add deep filter never to fix hand…

[jasong689]

Added a test to verify that you can still select the nested circular props. Also to note is that with DeepFilter you can no longer pass in a schema that has an optional property. This is fine with Gadget schemas as these are typed as prop: type | null instead of prop?: type

We will probably want to bump a minor version for this so that this change doesn't get picked up by older client packages

cc @angelini

PR Checklist

• Important or complicated code is tested
• Any user facing changes are documented in the Gadget-side changelog
• Any immediate changes are slated for release in Gadget via a generated package dependency bump
• Versions within this monorepo are matching and there's a valid upgrade path

[angelini]

Just a few notes re deployment:

• will need to increase the version of the package
• the ShipIt tool to update the version in everyone's environment currently requires too much disk space, we need to update the job to cleanup after itself.

[jasong689]

@angelini we won't need to rebuild everyone's client for this. just new ones being generated going forward

[thegedge]

If we remove this, there is a window of time where the generated API client for an app (potentially) uses this version of api-client-core but doesn't DeepFilterNever the return.

I think maybe we take a different approach: what if we give the current Select a new name, say InnerSelect, and instead do this:

export type Select<Schema, Selection extends FieldSelection | null | undefined> = DeepFilterNever<InnerSelect<Schema, Selection>>

which doesn't require a client rebuild and would only need a minor version bump (since it's backwards compatible).

Also, since the Select above has no constraints on the Schema type, we either need to consider adding the extends Record<string, unknown> constraint or making DeepFilterNever not need it, e.g.

export type DeepFilterNever<T> = T extends Record<string, unknown>
  ? FilterNever<{
      [Key in keyof T]:
         T[Key] extends Record<string, unknown>
         ? DeepFilterNever<T[Key]>
         : T[Key];
    }>
  : T
}>;

I think putting the constraint on Schema should be fine since a schema is never a literal / array / non-object thingy.

What do you think?

[thegedge]

And note, by "potentially uses", I think it would require the users to manually specify an api-client-core that would result in a yarn install grabbing the new version. Unlikely, but it could happen. If there's any concern there, we can double check existing package.jsons to see if this would happen.

[jasong689]

@thegedge that makes sense. One other thing that @airhorns brought up was that we don't yarn install on every rebuild so even though we've updated their api-client-core version the old version is the one that's imported until someone runs yarn install. So I think in addition to your suggested change, I'm going to re-implement Select in the same way in our codegen and remove it once all packages have been updated.

[thegedge]

Super happy this worked out! 🚀

[thegedge]

Might be worth expanding a bit by calling out that objects, after having their never-valued keys filtered out, also get filtered out of their parent if they are empty. i.e.,

type A = DeepFilterNever<{ a: { b: never }, c: "test" }>;
type B = { c: "test" };  // same as A
type C = { a: {}, c: "test" };  // not this one though

Suggesting that because an empty object is distinct from never. Worth a double check that it's doing this.

[jasong689]

Added an example in the comment and a test to make sure it's the case.

[jasong689]

Failing tests look be a real issue. Will address