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 RequireOnlyOne type #24

Closed
wants to merge 17 commits into from
Closed

Add RequireOnlyOne type #24

wants to merge 17 commits into from

Conversation

izayl
Copy link

@izayl izayl commented Apr 8, 2019

No description provided.

@CvX
Copy link
Collaborator

CvX commented Apr 8, 2019

I've just noticed you've changed never to undefined before. What was the issue with never?

@izayl
Copy link
Author

izayl commented Apr 9, 2019

@CvX
there is a test to prove that never is not suitable for some case.
15dc6dd

@CvX
Copy link
Collaborator

CvX commented Apr 9, 2019

Hmm… With that change though, it also incorrectly accepts this:

const system = {macos: 'hey', x: 1} || {linux: 'sup', y: 'wat'};
test({default: 'hello', ...system});

@izayl
Copy link
Author

izayl commented Apr 9, 2019

Hmm… With that change though, it also incorrectly accepts this:

const system = {macos: 'hey', x: 1} || {linux: 'sup', y: 'wat'};
test({default: 'hello', ...system});
type SystemMessages = {
	default: string;

	macos?: string;
	linux: string;
	windows: string;

	optional?: string;
};
const test = (_: ValidMessages): void => {};

@CvX Because the property x and y not in the type ValidMessages.

@CvX
Copy link
Collaborator

CvX commented Apr 9, 2019

That's my point. 🙂 With the current RequireOnlyOne implementation and the …({} || {}) construct you can pass pretty much anything and it doesn't raise an error.

@izayl
Copy link
Author

izayl commented Apr 9, 2019

you are right, current implementation RequireOnlyOne not support this Object deconstruction scene.

the case is correct, my editor highlight some error confused me, it works after restart my editor.
😢

@CvX
Copy link
Collaborator

CvX commented Apr 9, 2019

my editor highlight some error confused me, it works after restart my editor

Same here 😅 Turns out there isn't really any difference between never and undefined in this case?

See the commented-out test I've just pushed. It doesn't raise an error… 🤔

@izayl
Copy link
Author

izayl commented Apr 9, 2019

my editor highlight some error confused me, it works after restart my editor

Same here 😅 Turns out there isn't really any difference between never and undefined in this case?

See the commented-out test I've just pushed. It doesn't raise an error… 🤔

I think it's a bug cause by ts type checker.
The type { x?: never } says either the property does not exist, or it exists with type never. It's mean that x does not exist, though the type should really be isomorphic to never.

@sindresorhus
Copy link
Owner

I think it's a bug cause by ts type checker.

Can you look for an issue about this in the TS issue tracker and if not, open an issue there?


/*
Create a type that requires at only one of the given properties. The remaining properties are kept as is.

Copy link
Owner

Choose a reason for hiding this comment

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

Can you mention in text some real-world use-cases?

Write about some real-world use-cases where it can be useful. (It can be hard sometimes for users to see where they would use something) - https://github.com/sindresorhus/type-fest/blob/master/.github/contributing.md

@@ -198,3 +198,36 @@ export type RequireAtLeastOne<ObjectType, KeysType extends keyof ObjectType = ke
}[KeysType]
// …then, make intersection types by adding the remaining properties to each mapped type.
& Omit<ObjectType, KeysType>;

/*
Create a type that requires at only one of the given properties. The remaining properties are kept as is.
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
Create a type that requires at only one of the given properties. The remaining properties are kept as is.
Create a type that requires only one of the given properties, but not more. The remaining properties are kept as is.

?

};
```
*/
export type RequireOnlyOne<ObjectType, KeysType extends keyof ObjectType = keyof ObjectType> =
Copy link
Owner

Choose a reason for hiding this comment

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

"Require only one" makes me think that it requires only one, but doesn't make it clear that it disallows more than one. Maybe we should simply name it RequireOne?

@sindresorhus
Copy link
Owner

Can you fix the merge conflict? (Note that I have moved types to separate files in master).

@sindresorhus
Copy link
Owner

@izayl Ping :)

@WORMSS
Copy link

WORMSS commented Jul 10, 2019

I just wanted to say that I simplified this concept a little so I could use it to build up a type rather than modify an existing type.

type OnlyOne<T> = {
    [K in keyof T]: (
        Required<Pick<T, K>> & Partial<Record<Exclude<keyof T, K>, never>>
    )
}[keyof T];

Tested with the below code, when wrapped in Partial, it allows the group to also be optional or still only allow one.

type Test = OnlyOne<{
    a: boolean;
    b: boolean;
}> & Partial<OnlyOne<{
    c: boolean;
    d: boolean;
}>>;

const t1: Test = {
    a: true,
};
const t2: Test = {
    b: true,
};
const t3: Test = { // Should fail, a or b must be supplied
    
}
const t4: Test = { // Should fail a and b cannot be supplied together
    a: true,
    b: true,
}
const t5: Test = {
    a: true,
    c: true,
}
const t6: Test = { // Should fail c and d cannot be supplied together
    a: true,
    c: true,
    d: true,
}
const t7: Test = { // Should fail, as even though c is supplied, you still need a or b
    c: true,
}

I hope this helps anyone else in the future.

@sindresorhus sindresorhus mentioned this pull request Jul 26, 2019
@sindresorhus
Copy link
Owner

Closing for lack of response.

#54

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.

None yet

4 participants