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

Fix TypeScript types #92

Merged
merged 4 commits into from Feb 16, 2022
Merged

Fix TypeScript types #92

merged 4 commits into from Feb 16, 2022

Conversation

mmkal
Copy link
Contributor

@mmkal mmkal commented Jan 29, 2022

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.

@mmkal
Copy link
Contributor Author

mmkal commented Jan 29, 2022

@sindresorhus if you run the CI workflow we should see a ❌ , then I can push the fix so it can become a ✅

@mmkal
Copy link
Contributor Author

mmkal commented Jan 29, 2022

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.

@sindresorhus
Copy link
Owner

I would prefer using tsd where it works, but I'm ok with using expect-type for the assertions that are borked in tsd.

@mmkal
Copy link
Contributor Author

mmkal commented Feb 14, 2022

@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.

@sindresorhus
Copy link
Owner

Alright. Let's use expect-type.

Comment on lines +11 to +12
// @ts-expect-error type-fest's `Get` not smart enough to deal with escaped dots
).toEqualTypeOf<string>();
Copy link
Contributor Author

@mmkal mmkal Feb 15, 2022

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

@mmkal mmkal marked this pull request as ready for review February 15, 2022 13:32
@sindresorhus sindresorhus changed the title Switch to expect-type Fix TypeScript types Feb 16, 2022
@sindresorhus sindresorhus merged commit 2c1bbfb into sindresorhus:main Feb 16, 2022
@sindresorhus
Copy link
Owner

Thanks for fixing :)

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.

Examples don't seem to have correct typescript types
2 participants