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 input type on some methods and add AssertingValidator #224

Merged
merged 4 commits into from
Oct 4, 2021
Merged

Conversation

cpiber
Copy link
Contributor

@cpiber cpiber commented Sep 24, 2021

Fixes #222

  • Some functions incorrectly used value: T instead of value: unknown (want to assert value is T, not assume it)
  • Added type to make ReusableValidator also assert value is T
  • Convenience type to infer nested BasePredicate for object.partialShape (type-check that type and predicate are equal)

I am not fully sure about the last two points, suggestions welcome.

@cpiber
Copy link
Contributor Author

cpiber commented Sep 28, 2021

@sindresorhus Input?

@cpiber
Copy link
Contributor Author

cpiber commented Oct 1, 2021

While this isn't resolved, you can use

const is = <T>(v: unknown, pred: BasePredicate<T>): v is T => ow.isValid(v, pred);

to substitute ow.isValid

source/index.ts Outdated
}
export type AssertingValidator<T> = T extends ReusableValidator<infer R> ? (value: unknown, label?: string) => asserts value is R : never;
Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't the asserts be added to ReusableValidator?

source/utils/match-shape.ts Outdated Show resolved Hide resolved
source/index.ts Outdated
});
```
*/
export type ShapeOfType<T> = Infer<DeepShape<T>>;
Copy link
Owner

Choose a reason for hiding this comment

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

Why not improve the exactShape type instead of adding a separate type here?

@cpiber
Copy link
Contributor Author

cpiber commented Oct 3, 2021

For some reason, I cannot reply to the review, so here:

Shouldn't the asserts be added to ReusableValidator?

I tried that, but then you get Assertions require every name in the call target to be declared with an explicit type annotation.ts(2775) as mentioned before

Why not improve the exactShape type instead of adding a separate type here?

First of all, I didn't want to cause breaking changes, and second, I don't know if it is possible to do that, and if it is how. As I said, I introduced that type to make sure TypeScript actually verifies, that the shape of the argument and the type of what I want to verify against are the same, since simply declaring the target variable as ObjectValidator<> does not work. Please do suggest a better solution.

@sindresorhus
Copy link
Owner

I tried that, but then you get Assertions require every name in the call target to be declared with an explicit type annotation.ts(2775) as mentioned before

I would try to investigate that. Maybe there's a workaround. I don't have time to look into these issues now.

First of all, I didn't want to cause breaking changes, and second, I don't know if it is possible to do that, and if it is how. As I said, I introduced that type to make sure TypeScript actually verifies, that the shape of the argument and the type of what I want to verify against are the same, since simply declaring the target variable as ObjectValidator<> does not work. Please do suggest a better solution.

I unfortunately don't have time to look for a better solution.

I suggest removing the unrelated changes for now and focus on "Some functions incorrectly used value: T instead of value: unknown (want to assert value is T, not assume it)". The other things can be done later on in a separate pull request or opened as an issue instead if you cannot find a good solution for it.

@cpiber
Copy link
Contributor Author

cpiber commented Oct 3, 2021

I would try to investigate that. Maybe there's a workaround. I don't have time to look into these issues now.

I don't think it's possible due to the nature of ow.create: TS(2775) requires the variable to which ow.create's return is saved to to be explicitly typed from what I understand, I already tried to search for a better solution.

I suggest removing the unrelated changes for now

Understood, I'll remove them in a sec

@cpiber cpiber requested a review from sindresorhus October 3, 2021 18:32
@cpiber cpiber marked this pull request as ready for review October 3, 2021 18:32
@sindresorhus
Copy link
Owner

AssertingValidator should not be exported to the user. It's only meant for the tests here.

@cpiber
Copy link
Contributor Author

cpiber commented Oct 4, 2021

Is it? In my opinion, it should be exported, so that the user can have a validator, that actually asserts

@sindresorhus
Copy link
Owner

The intention for that was not clear. Then it needs a proper doc comment with a description (including why it exists) and a usage example.

@sindresorhus sindresorhus changed the title Typing value: unkown, convenience types Fix input type on some methods and add AssertingValidator Oct 4, 2021
@sindresorhus sindresorhus merged commit 4d3b55f into sindresorhus:main Oct 4, 2021
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.

Bug: isValid wrong typing
2 participants