Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SetRequiredDeep type #939

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hugomartinet
Copy link

@hugomartinet hugomartinet commented Aug 12, 2024

Closes #796

This pull request adds a new SetRequiredDeep which, like SetRequired, allows to make one or several keys in a model required. It adds the possibility to select nested keys by specifying their path.

Example

import type {SetRequiredDeep} from 'type-fest';

type Foo = {
	a?: number;
	b?: string;
	c?: {
		d?: number
	}[]
}

type SomeRequiredDeep = SetRequiredDeep<Foo, 'a' | `c.${number}.d`>;
// type SomeRequiredDeep = {
// 	a: number; // Is now required
// 	b?: string;
// 	c: {
//    d: number // Is now required
//  }[]
// }

@sindresorhus sindresorhus requested a review from Emiyaaaaa August 12, 2024 23:41
@Emiyaaaaa
Copy link
Collaborator

Emiyaaaaa commented Aug 14, 2024

Can this feature be added on SetRequired instead of a new type? just move code into SetRequired

@sindresorhus how about you think?

@sindresorhus
Copy link
Owner

The problem with doing both shallow and deep in a single method is that the deep logic affects the performance even if you don't need deep. This was the case for Simplify, where we had to split it out into a SimplifyDeep version.

source/set-required-deep.d.ts Show resolved Hide resolved
test-d/set-required-deep.ts Outdated Show resolved Hide resolved
@hugomartinet hugomartinet requested a review from Emiyaaaaa August 19, 2024 07:12
test-d/set-required-deep.ts Show resolved Hide resolved
test-d/set-required-deep.ts Show resolved Hide resolved
source/set-required-deep.d.ts Outdated Show resolved Hide resolved
@hugomartinet hugomartinet force-pushed the feat/SetRequiredDeep branch 2 times, most recently from 1c4f238 to e3c2874 Compare August 20, 2024 09:01
@hugomartinet hugomartinet requested a review from Emiyaaaaa August 20, 2024 09:03
@Emiyaaaaa
Copy link
Collaborator

Need to resolve all conversation

@hugomartinet
Copy link
Author

Need to resolve all conversation

Done

@sindresorhus sindresorhus requested review from Beraliv and removed request for Beraliv October 29, 2024 09:18
@Beraliv
Copy link

Beraliv commented Oct 29, 2024

@sindresorhus @hugomartinet I'll have a look at your PR later today or tomorrow


// Set nested key to required
declare const variation1: SetRequiredDeep<{a?: number; b?: {c?: string}}, 'b.c'>;
expectType<{a?: number; b: {c: string}}>(variation1);
Copy link

@Beraliv Beraliv Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You haven't specified 'b' | 'b.c', therefore the expected behaviour should be expectType<{a?: number; b?: {c: string}}>(variation1); (b is still optional)

Same below

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this remark. I actually did this on purpose, in my mind I thought that this is what we want.

Since SetRequired<..., 'b.c'> would mean that we want b.c to always be defined, hence we also need b to always be defined. Do you disagree ?

The reason is that I thought it would be fastidious when having nested objects with many levels. Writing SetRequired<..., 'a.b.${number}.c'> is lighter than writing SetRequired<..., 'a' | 'a.b' | 'a.b.${number}' | 'a.b.${number}.c'>.

What do you think ? If you're confident on making only the nested key required I can work on that again 😀


// Set key inside array to required
declare const variation7: SetRequiredDeep<{a?: Array<{b?: number}>}, `a.${number}.b`>;
expectType<{a: Array<{b: number}>}>(variation7);
Copy link

@Beraliv Beraliv Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar here (and below), the only required path `a.${number}.b` is specified, not 'a' | `a.${number}.b`, therefore the expected behaviour should be expectType<{a?: Array<{b: number}>}>(variation7);

@Beraliv
Copy link

Beraliv commented Oct 30, 2024

@hugomartinet I left my thoughts, I don't think it's ready because of a bug in the implementation (specified in my 2 comments where exactly), but I will leave it up to you (cc @sindresorhus)

@hugomartinet hugomartinet requested a review from Beraliv October 31, 2024 08:26
@hugomartinet
Copy link
Author

@hugomartinet I left my thoughts, I don't think it's ready because of a bug in the implementation (specified in my 2 comments where exactly), but I will leave it up to you (cc @sindresorhus)

@Beraliv I pushed a correction, I think I managed to take into account the expected behaviour you mentioned, thanks for pointing it out!

@hugomartinet
Copy link
Author

@sindresorhus @Beraliv How should we proceed on this?
Do you still have comments and suggestions?

@Beraliv
Copy link

Beraliv commented Dec 13, 2024

@hugomartinet thank you for your patience! If it waits, I'll leave my comments by the end of the year (next 2 weeks). If not, proceed with what you have and I'll raise bugs later if I find anything unexpected

Copy link
Contributor

@som-sm som-sm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hugomartinet The current implementation seems to fail in the following cases:

  1.  type T1 = SetRequiredDeep<{ a: 1; b: { c?: 1 } }, 'b.c'>;
     //   ^? type T1 = { a: 1; b?: { c: 1 } }

    Shouldn't make b optional.

  2.  type T1 = SetRequiredDeep<{ a: 1; readonly b: { c?: 1 } }, 'b.c'>;
     //   ^? type T1 = { a: 1; b?: { c: 1 } }

    Doesn't preserve the readonly modifier on b.

  3.  type T1 = SetRequiredDeep<{ a: 1; readonly b: { c?: 1 } | number }, 'b.c'>;
     //   ^? type T1 = { a: 1; b?: { c: 1 } }

    Doesn't preserve the | number on b.

  4.  type T1 = SetRequiredDeep<{ 0: 1; 1: { 2?: string } }, '1.2'>;
     //   ^? type T1 = {0: 1; 1: {2?: string}}

    Doesn't work with number keys.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SetRequiredDeep - is it possible?
5 participants