sindresorhus / type-fest

A collection of essential TypeScript types
Creative Commons Zero v1.0 Universal
14.03k stars 532 forks source link

`SetRequired` should return the original class type, plus the required keys #141

Open xenoterracide opened 3 years ago

xenoterracide commented 3 years ago

I don't really know if this is possible, but here's what I have

PaymentEntity is a class

type CardPaymentEntity = SetRequired<PaymentEntity, 'processor'>;

  static fromPayment(payment: CardPaymentEntity, opt: FromPayment): PaymentAuthorizationEntity {
    return new PaymentAuthorizationEntity(
      payment.amount,
      payment.currency,
      payment, // compile error here, because CardPaymentEntity is not a PaymentEntity class
Argument of type '{ readonly status: PaymentStatus; readonly amount: number; readonly currency: Currency; readonly orderId: string; readonly methodType: PaymentMethodType; ... 8 more ...; readonly processor: Processor | null; }' is not assignable to parameter of type 'PaymentEntity'.

Upvote & Fund

Fund with Polar

sindresorhus commented 3 years ago

Why should it return the original type of it's a class?

xenoterracide commented 3 years ago

well, perhaps I should not say the exact original type, but because the type changed to just a shape it no longer matched the class type for typescript, even though the only thing I wanted to do was make the processor field required at that point. AFAIK, a required field should be possible to pass into an optional field. Again, don't know if this is actually possible.

papb commented 3 years ago

Hmm, I see your point for SetRequired. If a type T is expected somewhere, passing SetRequired<T, 'whatever'> should also work. However I disagree with SetOptional and Except, because the new type would not fulfill the original "interface contract".

xenoterracide commented 3 years ago

ok, makes sense, just off the cuff those seemed like they should do the same last night. But after some sleep, you're right.

papb commented 3 years ago

@xenoterracide That said, I tried to create a small snippet to reproduce the problem but couldn't. I declared a class X and a function expecting X, then declared a variable with type SetRequired<X, 'foo'> and I could pass it to my function without any compiler error...

papb commented 3 years ago

Can you please modify this playground code I created to show the issue?

xenoterracide commented 3 years ago

I've got to be honest, right now I have no idea what I was doing last night that I created this issue, it was probably stupidly complex.

however, you might want to take a look at the playground, I'm actually surprised that it compiles given that the required field can be null | undefined

xenoterracide commented 3 years ago

ahah! got it, the SetRequired you pasted is not the same as the released version... or there's something else going on with it

playground

import { Except, SetRequired } from 'type-fest';

type FooRequired = SetRequired<X, 'foo' | 'bar' | 'baz'>;

class X {
  foo?: string;
  bar!: string | null;
  baz: undefined;
  private _createdAt = '';
}

function x1(x: X): void {
  x;
}

function x2(x: FooRequired): void {
  x1(x);
}

const a: FooRequired = {
  foo: 'true',
  bar: null,
  baz: undefined,
};

x2(a); // No error!

type BarExclude = Except<FooRequired, '_createdAt'>;

the actual problem seems to be private properties, also, you'll note that even though they're SetRequired bar and baz can still be set to null and undefined, this isn't what I would expect.

I seem to get one more compile error locally than on ts playground.

app-lib/graph/packages/app/src/payment/model/dal/test.ts:17:6 - error TS2345: Argument of type '{ foo: string; bar: string | null; baz: undefined; }' is not assignable to parameter of type 'X'.
  Property '_createdAt' is missing in type '{ foo: string; bar: string | null; baz: undefined; }' but required in type 'X'.

17   x1(x);
        ~

  app-lib/graph/packages/app/src/payment/model/dal/test.ts:9:11
    9   private _createdAt = '';
                ~~~~~~~~~~
    '_createdAt' is declared here.

app-lib/graph/packages/app/src/payment/model/dal/test.ts:28:39 - error TS2344: Type '"_createdAt"' does not satisfy the constraint '"foo" | "bar" | "baz"'.

28 type BarExclude = Except<FooRequired, '_createdAt'>;
                                         ~~~~~~~~~~~~

[10:41:44 PM] Found 3 errors. Watching for file changes.