-
-
Notifications
You must be signed in to change notification settings - Fork 98
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
Fix TypeScript types #92
Conversation
@sindresorhus if you run the CI workflow we should see a ❌ , then I can push the fix so it can become a ✅ |
Note: it might be possible to workaround the tsd issue. The error messages are a bit better in CI builds. Feel free to close this if you'd prefer to figure that out. |
I would prefer using |
@sindresorhus as far as I can tell, they are borked for any generic function. Borked in the sense that they don’t catch errors. So even though tsd only fails to catch the bug in dot-prop types in a couple of assertions, if the bug were more serious it would fail on the others. So at least for this library I don’t think it’s safe to use. |
Alright. Let's use |
// @ts-expect-error type-fest's `Get` not smart enough to deal with escaped dots | ||
).toEqualTypeOf<string>(); |
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 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 🤷 )
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 agree: #95
Thanks for fixing :) |
Fixes #84
First commit a3b3bad adds typescript, a tsconfig.json file and switches to from tsd to expect-type (due to tsdjs/tsd#142), to expose the problem in the test-d file.
Second commit will fix the bug in the typings.