refactor: Improve projection type safety

[joshuat]

Rather than using the built-in Partial utility, we're adding an ExactPartial utility that allows us to simulate the behavior of Partial while excluding extraneous properties.

BREAKING CHANGE: Changes arguments of Projection type to exclude extraneous properties

Closes: #683

[joshuat]

This will cause a merge conflict with #711 - if that PR is merged first, we'll need to update the projection type signature in this PR.

[avaly]

This looks amazing and so simple.

I wonder if we need to treat this as a breaking change since we changed the arguments to the Projection type, which is exported.

[avaly]

TIL that you can use the same generic identifier in both the left side and right side of the extends. How is that even possible? Isn't there some kind of recursion limit?

[joshuat]

Circular generic constraints are really interesting and make quite a bit of sense, but they only currently work when they're non-immediate. Definitely pushes the boundaries of what TypeScript is able to deliver.

Here is a really good thread if you want to read a bit more about them.

And there is a recursion limit, but it's pretty sizable and we'd hit that when building the schema path options already if we were going to hit it, so there's no additional risk here.

[avaly]

I just checked our private codebase and we definitely use Projection in a lot of places. So this will need to be marked as a breaking change.

[joshuat]

@avaly Rebased and marked as breaking 👍

[avaly]

Can you please combine some valid properties with this invalid one here?

[avaly]

@joshuat Here's a edge case with this change found in our private codebase:

async function doSomething<
  TProjection extends Projection<TProjection, FooDocument>
>(projection: TProjection): void {
  const items = await Foo.find(
    { identifier: { $in: ['foo', 'bar'] } },
    {
      projection: {
        ...projection,
        identifier: 1
      }
    }
  );
  // do something with items
}

This throws an error on the projection: { line with the error:

Type 'TProjection & { identifier: number; }' is not assignable to type 'ExactPartial<TProjection & { identifier: number; }, Record<"title" | "type" | "_id" | ... 61 more ... | `otherField`, number>>'

If I add & { identifier: 1 } next to the TProjection in the right side of the extends definition, then TS complains with Type parameter TProjection has a circular constraint.

Any clue if we can solve this case?

[avaly]

@joshuat I found another broken case from our private repo. Here's a simplified version of the code that breaks:

async function iterate<
  TDocument extends BaseSchema,
  TOptions extends SchemaOptions<TDocument>,
  TProjection extends Projection<TProjection, TDocument>
>(
  params: IterateParams<TDocument, TOptions, TProjection>,
  mutator: (doc: ProjectionType<TDocument, TProjection>) => unknown
): Promise<void> {}

await iterate(
  {
    match: { type: JobType.VIONLABS },
    model: Job
  },
  async job => {
    job.ref; // TS throws an error because it does not know `ref` (a valid property on the model)
  }
);

The actual error seems to be:

Property 'ref' does not exist on type 'WithId<Pick<{ _id: ObjectId; updatedAt: Date; type: JobType; createdAt: Date; payload?: Pick<{ [x: string]: NonNullable<string | number | boolean | Date | ObjectId | undefined> | undefined; }, string> | undefined; ref?: Pick<...> | undefined; }, "_id">>'.ts(2339)

It looks like without sending a projection parameter, TS tries to Pick<TDocument, '_id'>.

[joshuat]

@avaly Hmmm. Appreciate you surfacing these - we're not using Papr in this way, so I wouldn't have caught them myself. I'll work through these over the week but mostly over the weekend and add some test cases to make sure we're covered from all directions.

[joshuat]

I tried to resolve this a few times over the past few weeks but got nowhere - it seems like we're hitting a hard wall around exact type restrictions, specifically on non-"fresh" objects that already have a type assignment.

Once microsoft/TypeScript#12936 is resolved we can give this another try, but for now, I think we have to accept maybe we're at the limits of what we're able to enforce with TypeScript.

Sorry everyone - we got close!