-
-
Notifications
You must be signed in to change notification settings - Fork 575
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
Enhance Except
type
#560
Conversation
@sindresorhus I would appreciate your review. |
// @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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
source/except.d.ts
Outdated
import type {Except} from 'type-fest'; | ||
import {Except} from 'type-fest'; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
@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. |
@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? |
source/except.d.ts
Outdated
type ExceptOptions = { | ||
/** | ||
@default false | ||
*/ | ||
strict?: boolean; | ||
}; |
There was a problem hiding this comment.
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:
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; | |
}; |
source/except.d.ts
Outdated
@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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated 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); |
source/except.d.ts
Outdated
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>; |
There was a problem hiding this comment.
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:
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>> | |
: {}); |
There was a problem hiding this comment.
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.
// ...
*/
Which types use |
There was a problem hiding this 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.
Making it |
There was a problem hiding this 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.
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?
The trade-off should be documented. |
Changed the
Added documentation for the trade-off as well. |
source/except.d.ts
Outdated
/** | ||
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`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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`. |
There was a problem hiding this comment.
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 optionrequireExactProps: true
to disallow assigning additional properties to this type. (Note that that any omitted properties in the resulting type will be present in suggestions asundefined
.)
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...
...
*/
There was a problem hiding this comment.
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`.) |
There was a problem hiding this comment.
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.
source/except.d.ts
Outdated
/** | ||
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`. |
There was a problem hiding this comment.
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 totrue
.
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) |
There was a problem hiding this comment.
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.
Made the requested changes. |
Thank you! |
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 thestrict
option totrue
. The rationale for this change was in response to the feedback provided in issue #556, which requested a stricter version of theExcept
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.