-
Notifications
You must be signed in to change notification settings - Fork 61
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
feat: add validate-value #508
base: master
Are you sure you want to change the base?
Conversation
@RA80533 wanna rebase that with all of the recent changes and try again? |
e25b9f1
to
8dc8934
Compare
Seems to be the build step is failing. Is it working on your local?
Does this pass? |
The import for expect-type should be removed.
|
Why? It's being used for a type test. |
This reverts commit 32ae195.
Woah, I'm nuts. I thought I saw a commit that removed it as a dependency. |
The actual type test fails. It seems the test was written expecting it to evaluate at run-time but it actually evaluates the test at compile-time. // %inferred-type: any
res;
|
Yeah the type tests are compile time only, as there is no way to test types at runtime. |
Before your issue, I didn't have the build tests at all. It was just running jest. But I added the build step to GH Actions to catch type errors too. But this test was there before and it did actually fire via jest when there were issues. |
When I did this PR originally, I removed line 31 in order to get the tests to pass because of the failed expectation. |
Annotating |
Jest already runs the TypeScript compiler through ts-jest so the extra workflow step of calling |
Not redundant, because not all code is tested. As evident from your original issue - the benchmarking code was failing after an automatic upgrade.
But the point of having it as input: any I'll take a look at your PR code shortly. |
Ok, so the problem is that Per the docs of the lib, you want to use the
|
After a bit of digging, I found out why it was passing prior to my original issue. The expected behavior of ts-jest is to both type-check files and run the tests (see here), I was perplexed as to why those specific errors weren't showing up in the logs. Jest wasn't type-checking index.ts because it was not picked up by Jest (it is not in the Jest glob nor is it included by anything in the glob). Furthermore, it wasn't counted towards any coverage metrics because it had no exports.
Good catch—using the function with an explicit type parameter settles things down. I didn't realize types were being narrowed for |
validate() { | ||
const { data } = this; | ||
|
||
if (isOfType<Data>(data, dataType.schema)) { |
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 also don't want to type hint with Data
, because that defeats the purpose of DRY. The idea is to define the validation once, and from that TS can infer the type.
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.
TypeScript cannot infer the type isOfType()
should narrow data
to for the same reason it must be narrowed: data
is of type any
. Unfortunately, validate-value provides no type relation between dataType.schema
and any would-be valid objects conforming to the schema.
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.
TypeScript cannot infer the type
Really? But the docs say that it is a type guard... The only purpose of the type guard is to infer type. So are the docs wrong then?
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.
Hm, yeah, ok, I verified it with their example and type guarding indeed does not work. In which case then this library does not fit the parameters of this repository.
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.
See criteria: https://github.com/moltar/typescript-runtime-type-benchmarks#criteria
The idea is that we don't want to define the validation AND the type in two places. Types should be inferred from the schema. See all of the other test cases. In no other test case we use the Data
type to hint what the type is. The validation or type guarding function infers the type automatically.
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.
Really? But the docs say that it is a type guard... The only purpose of the type guard is to infer type. So are the docs wrong then?
It is a generic type guard where you must write the type you wish dataType.schema
to represent. The library does not evaluate what the expected type is on its own because it does not compute what objects conforming to dataType.schema
would look like. As a result, the only thing TypeScript understands when data
passes through the type guard is that the type of data
conforms to the schema dataType.schema
.
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.
It is a generic type guard
Fair.
But in that case it does not fit the purpose of this repository. So we should close this PR, or fix the library to do type guarding without generics hints.
Yup, so that's why I added the build step as well. The index benchmark code isn't really testable. |
validate-value