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

Enhance Except type #560

Merged
merged 28 commits into from Mar 23, 2023
Merged

Enhance Except type #560

merged 28 commits into from Mar 23, 2023

Conversation

orimiles5
Copy link
Contributor

@orimiles5 orimiles5 commented Feb 28, 2023

Closes #556.

Hello There!

I would like to introduce a PR that enhances the Except type to be more strict. Although the updated type remains non-strict by default, users have the option to use the strict type by setting the strict option to true. The rationale for this change was in response to the feedback provided in issue #556, which requested a stricter version of the Except type.

Additionally, I can make further modifications to the types that use the Except type, if you would like.

To assist with the adoption of the updated type, I have included some examples in the JSDoc. If you have any questions or feedback about this change, please do not hesitate to let me know.

Thank you for your time and consideration.

@orimiles5 orimiles5 closed this Feb 28, 2023
@orimiles5 orimiles5 changed the title Improve Except type Enhance Except type Mar 1, 2023
@orimiles5 orimiles5 reopened this Mar 1, 2023
@orimiles5 orimiles5 closed this Mar 1, 2023
@orimiles5 orimiles5 reopened this Mar 1, 2023
@orimiles5
Copy link
Contributor Author

@sindresorhus I would appreciate your review.

@sindresorhus
Copy link
Owner

// @pheis @tommy-mitchell @rayrw Would any of you be willing to help review this?

readme.md Outdated
@@ -902,4 +902,4 @@ You can find some examples in the [TypeScript docs](https://www.typescriptlang.o

## License

SPDX-License-Identifier: (MIT OR CC0-1.0)
SPDX-License-Identifier: (MIT OR CC0-1.0)
Copy link
Owner

Choose a reason for hiding this comment

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

Don't make unrelated changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't figure out what was the change, maybe it's a white space?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the trailing new line was changed.

Copy link
Owner

Choose a reason for hiding this comment

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

@orimiles5 This still needs to be fixed.

Copy link
Contributor Author

@orimiles5 orimiles5 Mar 22, 2023

Choose a reason for hiding this comment

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

@sindresorhus Could you give me some advice on how to fix this?

Comment on lines 41 to 73
import type {Except} from 'type-fest';
import {Except} from 'type-fest';
Copy link
Owner

Choose a reason for hiding this comment

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

Don't make unrelated changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@sindresorhus
Copy link
Owner

@orimiles5 Can you think of any reason to want the non-strict version? If not, we could plan to remove the option and default to strict in the next major version.

@tommy-mitchell
Copy link
Contributor

@sindresorhus are you suggesting keeping the default as non-strict in this PR, with an option to be strict? Or that the this PR should have an option to be non-strict, and that the next version will remove that option?

Comment on lines 34 to 39
type ExceptOptions = {
/**
@default false
*/
strict?: boolean;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Could add a description to the option:

Suggested change
type ExceptOptions = {
/**
@default false
*/
strict?: boolean;
};
type ExceptOptions = {
/**
Disallow assigning non-specified properties.
Setting this to `false` is not recommended. Included for backwards-compatability.
@default false
*/
strict?: boolean;
};

Comment on lines 28 to 32
@see {NonStrictExcept}
*/
type Filter<KeyType, ExcludeType> = IsEqual<KeyType, ExcludeType> extends true ? never : (KeyType extends ExcludeType ? never : KeyType);
type Filter<KeyType, ExcludeType> =
IsEqual<KeyType, ExcludeType> extends true ? never :
KeyType extends ExcludeType ? never : KeyType;
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated change.

Suggested change
@see {NonStrictExcept}
*/
type Filter<KeyType, ExcludeType> = IsEqual<KeyType, ExcludeType> extends true ? never : (KeyType extends ExcludeType ? never : KeyType);
type Filter<KeyType, ExcludeType> =
IsEqual<KeyType, ExcludeType> extends true ? never :
KeyType extends ExcludeType ? never : KeyType;
@see {Except}
*/
type Filter<KeyType, ExcludeType> = IsEqual<KeyType, ExcludeType> extends true ? never : (KeyType extends ExcludeType ? never : KeyType);

Comment on lines 48 to 114
The NonStrictExcept refers to the standard Except method that permits the assignment of an object with additional properties to an object of a different type.
@see https://github.com/sindresorhus/type-fest/issues/556

@category Object
*/
type NonStrictExcept<ObjectType, KeysType extends keyof ObjectType> = {
[KeyType in keyof ObjectType as Filter<KeyType, KeysType>]: ObjectType[KeyType];
};

/**
Create a type from an object type without certain keys - in stricter way.
When the "strict" option in ExceptOptions is true, StrictExcept will be used.

The StrictExcept method resolves the problem that arises when an object with additional properties is assigned to an object of a different type.
@see https://github.com/sindresorhus/type-fest/issues/556

@category Object
*/
type StrictExcept<ObjectType, KeysType extends keyof ObjectType> = NonStrictExcept<ObjectType, KeysType> & Partial<Record<KeysType, never>>;

/**
The Except type is the exported type, which determines the appropriate method to use (NonStrictExcept or StrictExcept) based on the options provided as the third argument (which is set to false by default).

@example
```
import type {Except} from 'type-fest';
import {Except} from 'type-fest';

type Foo = {
a: number;
b: string;
c: boolean;
};

type FooWithoutA = Except<Foo, 'a' | 'c'>;
//=> {b: string};
type FooWithoutA = Except<Foo, 'a', {strict: false}>; // False by default

const foo: Foo = {
a: 1,
b: 'b',
c: true,
};

const fooWithoutA: FooWithoutA = foo; // No error
//=> NonStrictExcept<Foo, 'a'>;

@example
```
import {Except} from 'type-fest';

@category Object
*/
export type Except<ObjectType, KeysType extends keyof ObjectType> = {
[KeyType in keyof ObjectType as Filter<KeyType, KeysType>]: ObjectType[KeyType];
type Foo = {
a: number;
b: string;
c: boolean;
};

type FooWithoutA = Except<Foo, 'a', {strict: true}>;

const foo: Foo = {
a: 1,
b: 'b',
c: true,
};

const fooWithoutA: FooWithoutA = foo; // Error
//=> StrictExcept<Foo, 'a'>;
*/
export type Except<ObjectType, KeysType extends keyof ObjectType, Options extends ExceptOptions = {strict: false}> =
Options['strict'] extends false ? NonStrictExcept<ObjectType, KeysType> : StrictExcept<ObjectType, KeysType>;
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for extra types, we can just inline:

Suggested change
The NonStrictExcept refers to the standard Except method that permits the assignment of an object with additional properties to an object of a different type.
@see https://github.com/sindresorhus/type-fest/issues/556
@category Object
*/
type NonStrictExcept<ObjectType, KeysType extends keyof ObjectType> = {
[KeyType in keyof ObjectType as Filter<KeyType, KeysType>]: ObjectType[KeyType];
};
/**
Create a type from an object type without certain keys - in stricter way.
When the "strict" option in ExceptOptions is true, StrictExcept will be used.
The StrictExcept method resolves the problem that arises when an object with additional properties is assigned to an object of a different type.
@see https://github.com/sindresorhus/type-fest/issues/556
@category Object
*/
type StrictExcept<ObjectType, KeysType extends keyof ObjectType> = NonStrictExcept<ObjectType, KeysType> & Partial<Record<KeysType, never>>;
/**
The Except type is the exported type, which determines the appropriate method to use (NonStrictExcept or StrictExcept) based on the options provided as the third argument (which is set to false by default).
@example
```
import type {Except} from 'type-fest';
import {Except} from 'type-fest';
type Foo = {
a: number;
b: string;
c: boolean;
};
type FooWithoutA = Except<Foo, 'a' | 'c'>;
//=> {b: string};
type FooWithoutA = Except<Foo, 'a', {strict: false}>; // False by default
const foo: Foo = {
a: 1,
b: 'b',
c: true,
};
const fooWithoutA: FooWithoutA = foo; // No error
//=> NonStrictExcept<Foo, 'a'>;
@example
```
import {Except} from 'type-fest';
@category Object
*/
export type Except<ObjectType, KeysType extends keyof ObjectType> = {
[KeyType in keyof ObjectType as Filter<KeyType, KeysType>]: ObjectType[KeyType];
type Foo = {
a: number;
b: string;
c: boolean;
};
type FooWithoutA = Except<Foo, 'a', {strict: true}>;
const foo: Foo = {
a: 1,
b: 'b',
c: true,
};
const fooWithoutA: FooWithoutA = foo; // Error
//=> StrictExcept<Foo, 'a'>;
*/
export type Except<ObjectType, KeysType extends keyof ObjectType, Options extends ExceptOptions = {strict: false}> =
Options['strict'] extends false ? NonStrictExcept<ObjectType, KeysType> : StrictExcept<ObjectType, KeysType>;
@example
```
import type {Except} from 'type-fest';
type Foo = {
a: number;
b: string;
};
type FooWithoutA = Except<Foo, 'a'>;
//=> {b: string}
const fooWithoutA: FooWithoutA = {a: 1, b: '2'};
//=> errors: 'a' does not exist in type '{ b: string; }'
type FooWithoutB = Except<Foo, 'b', {strict: true}>;
//=> {a: number} & Partial<Record<"b", never>>
const fooWithoutB: FooWithoutB = {a: 1, b: '2'};
//=> errors at 'b': Type 'string' is not assignable to type 'undefined'.
```
@category Object
*/
export type Except<ObjectType, KeysType extends keyof ObjectType, Options extends ExceptOptions = {strict: false}> = {
[KeyType in keyof ObjectType as Filter<KeyType, KeysType>]: ObjectType[KeyType];
} & (Options['strict'] extends true
? Partial<Record<KeysType, never>>
: {});

Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't allow me to make a suggestion there, but in the Except doc comment:

/**
Create a type from an object type without certain keys.

+ Use option `strict: true` to disallow assigning additional properties to this type. 

// ...
*/

@tommy-mitchell
Copy link
Contributor

Additionally, I can make further modifications to the types that use the Except type, if you would like.

Which types use Except?

Copy link
Contributor

@tommy-mitchell tommy-mitchell left a comment

Choose a reason for hiding this comment

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

I think this PR could be simpler.

@sindresorhus
Copy link
Owner

are you suggesting keeping the default as non-strict in this PR, with an option to be strict? Or that the this PR should have an option to be non-strict, and that the next version will remove that option?

Making it {strict: true} by default is a breaking change, so the option is required for now. I'm asking whether there are any good use-cases for having it as false. If not, we can consider switching it to true by default in the next major release and then removing the option.

Copy link
Contributor

@rayrw rayrw left a comment

Choose a reason for hiding this comment

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

I personally think the definition of strictness in this type could be more than one depending on which criteria do we prioritize (check if assignable v.s. accessible).

Yes, using the current Except type cannot prevent us from assigning objects with excess properties because of TypeScript's design decision. However, the default behavior actually can check if we can access to that property.

Taking the same fooWithoutA variable in the original issue #556 as an example. When we try to access fooWithoutA.a, we get this error.

Property 'a' does not exist on type 'Except<Foo, "a">'.(2339)

With the strict option in this PR turned on, we cannot assign foo to fooWithoutA anymore. However, this comes with a trade off. We have to make a accessible. fooWithoutA.a is now valid and is shown in auto-complete.

Screen Shot 2023-03-11 at 22 20 06

Screen Shot 2023-03-11 at 22 12 30

So far I cannot find a perfect solution that can prevent both, but I wouldn't say either one is wrong. For now, maybe an explicit option like makeExceptPropertiesUndefined is more accurate? What do you think?

@sindresorhus
Copy link
Owner

With the strict option in this PR turned on, we cannot assign foo to fooWithoutA anymore. However, this comes with a trade off. We have to make an accessible. fooWithoutA.a is now valid and is shown in auto-complete.

The trade-off should be documented.

@orimiles5
Copy link
Contributor Author

orimiles5 commented Mar 14, 2023

@sindresorhus I think the option should have a specific name for what exactly it's enabling. strict is confusing as the type is already a strict version of Omit.

Changed the strict option to requireExactProperties to better reflect its behavior.
If you have a better name suggestion, please advise.

The trade-off should be documented.

Added documentation for the trade-off as well.

@orimiles5 orimiles5 requested review from tommy-mitchell and sindresorhus and removed request for sindresorhus and tommy-mitchell March 14, 2023 17:08
/**
Create a type from an object type without certain keys.

Use option `requireExactProps: true` to disallow assigning additional properties to this type. Notice that any omitted properties in the resulting type will be present as `undefined`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Use option `requireExactProps: true` to disallow assigning additional properties to this type. Notice that any omitted properties in the resulting type will be present as `undefined`.
Use option `requireExactProps: true` to disallow assigning additional properties to this type. Note that any omitted properties in the resulting type will be present in suggestions as `undefined`.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could also add a note like this to the readme:


  • Exact - Create a type that does not allow extra properties. Use option requireExactProps: true to disallow assigning additional properties to this type. (Note that that any omitted properties in the resulting type will be present in suggestions as undefined.)

Which means line 46 could probably just be appended to line 44:

/**
Create a type from an object type without certain keys. Use option `requireExactProps: true` to disallow assigning additional properties to this type. Notice that any omitted properties in the resulting type will be present as `undefined`.

This type is a stricter...

...
*/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

readme.md Outdated
@@ -129,7 +129,8 @@ Click the type names for complete docs.

- [`EmptyObject`](source/empty-object.d.ts) - Represents a strictly empty plain object, the `{}` value.
- [`IsEmptyObject`](source/empty-object.d.ts) - Returns a `boolean` for whether the type is strictly equal to an empty plain object, the `{}` value.
- [`Except`](source/except.d.ts) - Create a type from an object type without certain keys. This is a stricter version of [`Omit`](https://www.typescriptlang.org/docs/handbook/utility-types.html#omittype-keys).
- [`Except`](source/except.d.ts) - Create a type from an object type without certain keys. Use option `requireExactProps: true` to disallow assigning additional properties to this type. (Note that that any omitted properties in the resulting type will be present in suggestions as `undefined`.)
Copy link
Owner

Choose a reason for hiding this comment

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

The description here should not be changed. The option is explained in the type docs. That's enough.

/**
Create a type from an object type without certain keys.
Create a type from an object type without certain keys. Use option `requireExactProps: true` to disallow assigning additional properties to this type. Note that any omitted properties in the resulting type will be present in suggestions as `undefined`.
Copy link
Owner

Choose a reason for hiding this comment

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

This text should be in the docs for ExceptOptions instead. The only text we need here is just:

We recommend setting the requireExactProps option to true.

readme.md Outdated
@@ -902,4 +902,4 @@ You can find some examples in the [TypeScript docs](https://www.typescriptlang.o

## License

SPDX-License-Identifier: (MIT OR CC0-1.0)
SPDX-License-Identifier: (MIT OR CC0-1.0)
Copy link
Owner

Choose a reason for hiding this comment

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

@orimiles5 This still needs to be fixed.

@orimiles5
Copy link
Contributor Author

orimiles5 commented Mar 22, 2023

Made the requested changes.
Let me know if further changes are required.

@sindresorhus sindresorhus merged commit c5743c9 into sindresorhus:main Mar 23, 2023
7 checks passed
@orimiles5
Copy link
Contributor Author

Thank you!

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.

Except is not strict
4 participants