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 option allowUndefinedInArrays to PartialDeep #1019

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

PicoI2
Copy link

@PicoI2 PicoI2 commented Dec 23, 2024

Add option allowUndefinedInArrays to PartialDeep, default to true to respect backward compatibility.

PartialDeep with recurseIntoArrays option works like this

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

interface Settings {
	languages: string[];
}

const partialSettings: PartialDeep<Settings, {recurseIntoArrays: true}> = {
	languages: [undefined]
};

But it can be annoying that undefined type could be allowed in an array of string.
So with this new option allowUndefinedInArrays: false, the languages array is still optional, but type undefined is not allowed anymore into the languages array.

@som-sm
Copy link
Collaborator

som-sm commented Dec 23, 2024

Will this create confusion with tuples, because allowUndefinedInArrays wouldn't do anything with tuples.

For eg,

// @exactOptionalPropertyTypes: false

type Test = PartialDeep<
  // ^? { a?: [string?] }
  {a: [string]},
  {recurseIntoArrays: true; allowUndefinedInArrays: false}
>;

const test: Test = {a: [undefined]};

In the example above, undefined is allowed even though allowUndefinedInArrays is set to false. And this is because allowUndefinedInArrays doesn't seem to do anything for tuples, for tuples, allowance of undefined is decided by exactOptionalPropertyTypes setting.

@PicoI2 WDYT?

@som-sm
Copy link
Collaborator

som-sm commented Dec 23, 2024

IMO, there's no concept of optionality with non-tuple arrays. A string[] is also in a way optional, because it either holds strings or nothing, so (string | undefined)[] isn't IMO the optional version of string[].

Although, adding the optional modifier (?) in a mapped type and instantiating it with a non-tuple array like T[], does produce (T | undefined)[], like Partial<string[]> gives (string | undefined)[], but this feels incorrect to me.

So, IMO, allowUndefinedInArrays in arrays should be the default behaviour.

@PicoI2
Copy link
Author

PicoI2 commented Dec 25, 2024

type Test = PartialDeep<
  // ^? { a?: [string?] }
  {a: [string]},
  {recurseIntoArrays: true; allowUndefinedInArrays: false}
>;

const test: Test = {a: [undefined]};

You are right, I did not saw this case. But as the option is allowUndefinedInArraysand not allowUndefinedInArraysAndTuples it could give a hint. Or maybe we could found another name for the option to be more explicit.

IMO, there's no concept of optionality with non-tuple arrays. A string[] is also in a way optional, because it either holds strings or nothing, so (string | undefined)[] isn't IMO the optional version of string[].

Although, adding the optional modifier (?) in a mapped type and instantiating it with a non-tuple array like T[], does produce (T | undefined)[], like Partial<string[]> gives (string | undefined)[], but this feels incorrect to me.

So, IMO, allowUndefinedInArrays in arrays should be the default behaviour.

I totally agree, but making allowUndefinedInArrays default value to false would be a breaking change, and nobody likes breaking changes. IMO, the default value should be true, until the release of the version 5 of type-fest, where it could be noticed in the migration guide.

@som-sm
Copy link
Collaborator

som-sm commented Dec 26, 2024

I totally agree, but making allowUndefinedInArrays default value to false would be a breaking change, and nobody likes breaking changes. IMO, the default value should be true, until the release of the version 5 of type-fest, where it could be noticed in the migration guide.

Yeah true, setting the default value to false would indeed be a breaking change. However, my rationale was that most users would likely be using the recurseIntoArrays option to make the items within the array optional, rather than adding undefined to the array itself, so it wouldn't be noticeable in majority of the cases.

But yeah, to be on the safer side we can keep it as an option, and in v5 we can either totally remove the option or flip the default value.

You are right, I did not saw this case. But as the option is allowUndefinedInArraysand not allowUndefinedInArraysAndTuples it could give a hint. Or maybe we could found another name for the option to be more explicit.

How about addUndefinedToNonTupleArrays or allowUndefinedInNonTupleArrays?

source/partial-deep.d.ts Outdated Show resolved Hide resolved
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.

2 participants