sindresorhus / type-fest

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

`PickDeep` strips `null` #880

Open relsunkaev opened 4 months ago

relsunkaev commented 4 months ago

If you have following types

type Owner = {
  name: string;
  age: number;
};
type Pet = {
  name: string;
  age: number;
  owner: Owner | null;
};

the resulting type of PickDeep<Pet, "name" | "owner.name"> is

type Result = {
    name: string;
    // `owner` is no longer nullable
    owner: {
        name: string;
    };
}

Upvote & Fund

Fund with Polar

sindresorhus commented 4 months ago

Confirmed.

Playground: https://www.typescriptlang.org/play/?#code/JYWwDg9gTgLgBAbwArAMYGsAiBTbYC+cAZlBCHAOQwCeY2AtEdgM4wUDcAUJzXXAPIB3AHbYocALyJOcOMICGIbAC44rKMGEBzLrPlaVcgK4gARmK74uPWtjhJs8KQhlzFh9Zp2v9h4SfMoXTgIETFVIVFxAB9jABs4y2teOyIICEl7NCxcMAAeBxgAGjgAIgUlUrhY0tCogDoK7FKAPnYgA

Emiyaaaaa commented 4 months ago

Thanks report, but I don't think this is a bug

Let's look some cases:

case 1

type Pet = {
 owner: { name: 'name1' }
      | { name: 'name2' ; foo: '' };
};
type Result = PickDeep<Pet, 'owner.name'>;

PickDeep should pick every union type, so result should indeed be

type Result = {
 owner: { name: 'name1' } | { name: 'name2' }
}

case 2

type Pet = {
 owner: { name: 'name1' }
      | { foo: '' };
};
type Result = PickDeep<Pet, 'owner.name'>;

PickDeep can not find key "name" in { foo: '' }, so result is

type Result = {
 owner: { name: 'name1' }
}

case 3

type Pet = {
 owner: { name: 'name1' }
      | null;
};
type Result = PickDeep<Pet, 'owner.name'>;

so according to case 2, PickDeep can not find key "name" in null, so result should be

type Result = {
 owner: { name: 'name1' }
}

This is the same as the current behavior

relsunkaev commented 4 months ago

That makes sense as to why it behaves like this but to me it seems like unexpected or maybe less ergonomic behavior.

My current use case is narrowing types on function inputs like so

type Owner = {
  name: string;
  age: number;
}

type Pet = {
  name: string;
  age: number;
  owner: Owner | null;
}

// I only use `name` and `owner.name`
function printGreeting(pet: PickDeep<Pet, "name" | "owner.name"> {
  if (pet.owner) {
    console.log(`Hi, my name is ${pet.name} and my owner is ${pet.owner.name}!`);
  } else {
    console.log(`Hi, my name is ${pet.name} and I'm looking for an owner!`);
  }
}

Right now the function would require pet to have an owner even though in the definition of Pet the owner can be null. This was unexpected because in my understanding the purpose of Pick is to narrow a type while PickDeep "changes" it.

Emiyaaaaa commented 4 months ago

Okay, you convinced me. maybe we should keep union in PickDeep to ensure type safety.

What do you think? @sindresorhus

sindresorhus commented 4 months ago

What would "case 2" become if so?

relsunkaev commented 4 months ago

@sindresorhus I actually thought about this a bit when writing my response. This behavior is not consistent with how Pick or property access on union types works in TypeScript and can also be considered unintuitive:

type Pet = {
  name: string;
  owner: { name: 'name1' } | { foo: '' };
};

declare const pet: Pet;

function onlyNeedsOwnerName(pet: PickDeep<Pet, "owner.name">) {
  ...
}

onlyNeedsOwnerName(pet)
// ^ will result in an error

Again, in this case PickDeep changes the type. Pet is not compatible with PickDeep<Pet, "owner.name"> which some people may find surprising.

Emiyaaaaa commented 4 months ago

What would "case 2" become if so?

that will be

type Result = {
 owner: { name: 'name1' } | { foo: "" }
}

It's look like confused, I am beginning to waver on whether need do this for "type safety"