-
Notifications
You must be signed in to change notification settings - Fork 65
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] Mark only imported types as optional in validation #204
Conversation
@schiller-manuel: this should fix #203 |
can't we import locally referenced files in the test so that types imported from there are not converted to |
I'm not sure to follow: export type Villain = {
name: string;
}
export type Citizen = {
villain: Villain | null;
}; is converted to: // Generated by ts-to-zod
import { z } from "zod";
export const villainSchema = z.object({
name: z.string()
});
export const citizenSchema = z.object({
villain: villainSchema.nullable()
}); and nothing is inferred from With regards to imported types, there are converted to Are you suggesting to add them to the Virtual File System created during the validation step? |
yes exactly, sorry for my unclear formulation. |
As we're transforming import { Hero } from "./hero-module.ts"
export interface Citizen {
hero: Hero
}; into import { z } from "zod";
const heroSchema = z.any();
export const citizenSchema = z.object({
hero: heroSchema
}); we would still have an Even if, in the test, we add Maybe you have a different suggestion that I'm not seeing? I understand my fix is not ideal: this whole |
I thought we could distinguish between
for 1. we could add those files into the VFS, for 2. we would not. |
In that case, we would still need a way to mark those from 2 as optional during the validation, due to the bug mentioned in #140 |
yes. do you think we gain anything / much in terms of typesafety during validation from this? |
It's probably better yes, I'll have a look this afternoon (CET) |
I added the extraFiles parameter in this commit: 4c41597 but the test doesn't pass, if you have any idea. I'm looking at the "should return no error if we reference a zod import" test and I get:
and type output in the console is type CitizenInferredType = {
[x: string]: any;
hero?: any;
} It seems that the zod schema is not handled well by the VFS. |
The diff --git a/src/core/validateGeneratedTypes.test.ts b/src/core/validateGeneratedTypes.test.ts
index 5f62316..5068c88 100644
--- a/src/core/validateGeneratedTypes.test.ts
+++ b/src/core/validateGeneratedTypes.test.ts
@@ -411,7 +411,8 @@ describe("validateGeneratedTypes", () => {
relativePath: "hero-module.ts",
},
{
- sourceText: `export const heroSchema = z.object({ name: z.string() })`,
+ sourceText: `import { z } from "zod";
+ export const heroSchema = z.object({ name: z.string() })`,
relativePath: "zHero.ts",
},
]; |
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #204 +/- ##
==========================================
+ Coverage 97.20% 97.26% +0.05%
==========================================
Files 15 16 +1
Lines 823 841 +18
Branches 334 341 +7
==========================================
+ Hits 800 818 +18
Misses 23 23 ☔ View full report in Codecov by Sentry. |
@tvillaren let me know when this is ready to review |
@schiller-manuel My solution for now is to add fake folders in the VFS based on the maximum depth we're navigating, but I'm not handling all cases yet... |
sounds ... hacky 🙃 |
Let's imagine you run This If you add import { z } from "zod";
export const heroSchema = z.object({ name: z.string() })`, without a change, it seems to me that the VFS folder would look like:
so the import of If we add extra folders, based on the maximum depth, we get:
which works well. Yes, it does sound hacky. Right now, I managed to get the tests to pass but not yet the |
beafdb1
to
7edc41e
Compare
OK, the last version I pushed should cover the case of #207 (which I also added in the example provided through Right now, we're passing all the files from the Also, the CI passes even when there are errors in the |
sidenote: exiting with code 1 on error might be a breaking change ... so we should increase to v4 maybe? |
Full quote since there were messages in between:
would it help if the source.ts file was placed at the location of the input file in the VFS? would that result in correct relative links? |
Indeed, maybe we should change this in another PR, as this PR is meant to fix a bug :) |
You mean recreating the whole folder structure "above" the source & generated files from the original project in the VFS? This means we would need the "deepest" of both source & generated file path (as there might be relative imports in both) |
Why do we need to "recreate" the existing folder / file structure at all? |
You're right, I was not too familiar with VFS upon writing. And we indeed use But as the 3 files used for validation are currently put in the root folder of the VFS (as |
let me know when it is ready to review please |
This reverts commit 95ecc61.
ffee712
to
24aa539
Compare
@schiller-manuel: I believe it is ready for review 👍 |
src/utils/getImportPath.ts
Outdated
const normalizePath = (path: string) => { | ||
const { dir, name } = parse(normalize(path)); | ||
// Ensure directory path ends with a separator for consistency | ||
const dirWithSeparator = dir ? `${dir}/` : ""; |
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.
does this slash need to take platform specifics into account ( e.g. maybe on Windows it should be a \
)?
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.
This is only meant to handle import paths, not absolute ones.
Can import paths have backslashes? I really don't know 🤷♂️
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.
how about using path.sep
instead of a hardcoded /
?
see https://nodejs.org/api/path.html#pathsep
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.
or even better, do not manually build the path, but instead use path.join
:
return join(dir, name);
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.
Done ✅
very cool, thanks a lot |
Why
Following last release which added import handling, all type reference nodes were marked as optional in the validation step while this was only needed for imported type reference (which are inferred as
any
by Typescript, generated the bug fixed in #140)Fixes #203