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.

Sign up for GitHub

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

Jump to bottom

Fix TypeScript types #92

Merged
merged 4 commits into from
Feb 16, 2022
Merged

Fix TypeScript types #92

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a3b3bad
Switch to expect-type
mmkal Jan 29, 2022
65ee3be
Update tsconfig.json
sindresorhus Feb 15, 2022
9fda35d
Update package.json
sindresorhus Feb 15, 2022
a26ee10
Fix the bug
mmkal Feb 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ export function getProperty<ObjectType, PathType extends string, DefaultValue =
object: ObjectType,
path: PathType,
defaultValue?: DefaultValue
): ObjectType extends Record<string, unknown> | unknown[] ? (Get<ObjectType, PathType> extends unknown ? DefaultValue : Get<ObjectType, PathType>) : undefined;
): ObjectType extends Record<string, unknown> | unknown[] ? (unknown extends Get<ObjectType, PathType> ? DefaultValue : Get<ObjectType, PathType>) : undefined;

/**
Set the property at the given path to the given value.
Expand Down
21 changes: 11 additions & 10 deletions index.test-d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,19 @@
import {expectType, expectAssignable} from 'tsd';
import {expectTypeOf} from 'expect-type';
import {getProperty, setProperty, hasProperty, deleteProperty} from './index.js';

expectType<string>(getProperty({foo: {bar: 'unicorn'}}, 'foo.bar'));
expectType<undefined>(getProperty({foo: {bar: 'a'}}, 'foo.notDefined.deep'));
expectAssignable<string>(
expectTypeOf(getProperty({foo: {bar: 'unicorn'}}, 'foo.bar')).toBeString();
expectTypeOf(getProperty({foo: {bar: 'a'}}, 'foo.notDefined.deep')).toBeUndefined();
expectTypeOf(
getProperty({foo: {bar: 'a'}}, 'foo.notDefined.deep', 'default value'),
);
expectType<string>(
).toBeString();
expectTypeOf(
getProperty({foo: {'dot.dot': 'unicorn'}}, 'foo.dot\\.dot'),
);
// @ts-expect-error type-fest's `Get` not smart enough to deal with escaped dots
).toEqualTypeOf<string>();
Comment on lines +11 to +12
Copy link
Contributor Author

@mmkal mmkal Feb 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@sindresorhus worth noting - when unsure what the return type would be, it defaults to undefined. The "default" in type-fest.Get is unknown, since typescript in general allows for additional properties, so we can't know for sure it's undefined. It seems that getProperty effectively overrides the behaviour by making the default DefaultValue undefined - IMO it shouldn't.

I want to keep this PR focused so I'm not proposing to change that here but I think it should be considered to switch to unknown (obviously best solution is to make Get respect \. but that would make it incompatible with lodash.get so 🤷 )

Copy link
Owner

Choose a reason for hiding this comment

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

I agree: #95


const object = {foo: {bar: 'a'}};
expectType<typeof object>(setProperty(object, 'foo.bar', 'b'));
expectTypeOf(setProperty(object, 'foo.bar', 'b')).toEqualTypeOf(object);

expectType<boolean>(hasProperty({foo: {bar: 'unicorn'}}, 'foo.bar'));
expectTypeOf(hasProperty({foo: {bar: 'unicorn'}}, 'foo.bar')).toEqualTypeOf<boolean>();

expectType<boolean>(deleteProperty({foo: {bar: 'a'}}, 'foo.bar'));
expectTypeOf(deleteProperty({foo: {bar: 'a'}}, 'foo.bar')).toEqualTypeOf<boolean>();
5 changes: 3 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
"node": "^12.20.0 || ^14.13.1 || >=16.0.0"
},
"scripts": {
"test": "xo && ava && tsd",
"test": "xo && ava && tsc",
"bench": "node benchmark.js"
},
"files": [
Expand All @@ -42,7 +42,8 @@
"devDependencies": {
"ava": "^4.0.1",
"benchmark": "^2.1.4",
"tsd": "^0.19.1",
"expect-type": "^0.13.0",
"typescript": "^4.5.5",
"xo": "^0.47.0"
}
}
12 changes: 12 additions & 0 deletions tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
{
"compilerOptions": {
"lib": [
"es2020"
],
"strict": true,
"noEmit": true
},
"include": [
"*.ts"
]
}