[Fix] Mark only imported types as optional in validation

[tvillaren]

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

[tvillaren]

@schiller-manuel: this should fix #203
What do you think?

[schiller-manuel]

can't we import locally referenced files in the test so that types imported from there are not converted to any?
e.g. the Person interface in the example?

[tvillaren]

can't we import locally referenced files in the test so that types imported from there are not converted to any?

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 any here
👉 that is the current issue (in the validation, we handle any referenced types, even non-imported ones, as any

With regards to imported types, there are converted to any by ts-to-zod as a choice in my previous PR because we cannot transform all the imports to zod.

Are you suggesting to add them to the Virtual File System created during the validation step?
I remember fiddling with that in vain a while ago when I had the validation issues with imports (#186)

[schiller-manuel]

Are you suggesting to add them to the Virtual File System created during the validation step?

yes exactly, sorry for my unclear formulation.
your fix is good, but it would be even better if during validation the other files would be imported using the VFS.

[tvillaren]

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 any type on the Zod-inferred one, which will be inferred as optional (not a new issue, a known bug as exposed in #140)

Even if, in the test, we add hero-module.ts to the VFS, it doesn't work (I had a few tries in the context of my last PR, see 954bbcf that passed only because we were using class and wouldn't with type)

Maybe you have a different suggestion that I'm not seeing?

I understand my fix is not ideal: this whole fixOptionalAny is not TBH 😅 but is needed by #140
An alternative would be to resolve the Symbol linked to each TypeReference node in the fixOptionalAny to check if it is imported or not in the file.

[schiller-manuel]

I thought we could distinguish between

1. types that are imported from files which ARE part of the ts-to-zod config and thus create schemas for them and
2. types that are imported from files which are NOT part of the ts-to-zod config and thus we treat them with z.any() ("external imports")

for 1. we could add those files into the VFS, for 2. we would not.

[tvillaren]

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

[schiller-manuel]

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?
if not, I will just merge this PR as is.

[tvillaren]

It's probably better yes, I'll have a look this afternoon (CET)

[tvillaren]

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:

 "'Citizen' is not compatible with 'Citizen':
    + Argument of type '{ [x: string]: any; hero?: any; }' is not assignable to parameter of type 'Citizen'.
    +   Property 'hero' is optional in type '{ [x: string]: any; hero?: any; }' but required in type 'Citizen'.",

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.
Do you see anything?

[schiller-manuel]

The zod import was missing in zHero.ts, test passes if I add is like this:

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-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.26%. Comparing base (34e76f2) to head (32f6b24).

❗ 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.
📢 Have feedback on the report? Share it here.

[schiller-manuel]

@tvillaren let me know when this is ready to review

[tvillaren]

@schiller-manuel
Handling relative paths with parent directory accessors ../ in the VFS is a bit tricky, particularly when the added file references imports from the node_modules (which are at the root of the VFS, so if we add files to the parent directory of the root, well, it doesn't find the import...).

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...
Will get there eventually 😉

[schiller-manuel]

My solution for now is to add fake folders in the VFS based on the maximum depth we're navigating

sounds ... hacky 🙃
can you explain in detail please why these relative imports are problematic?
I thought we would "simply" add the existing files into the VFS and that's it. what am I missing?

[tvillaren]

Let's imagine you run ts-to-zod in /Users/me/ts-to-zod

This pwd becomes the root for the VFS.
That's were the node_modules folder will be set, as well as the source / schema / integration files.

If you add ../zHero.ts as extra files with content

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:

- Users
   - me
      - zHero.ts
      - ts-to-zod
         - node_modules
         - source.ts
         - source.zod.ts
         - source.integration.ts

so the import of zod in zHero.ts doesn't work.

If we add extra folders, based on the maximum depth, we get:

- Users
   - me
      - ts-to-zod
         - node_modules
         - zHero.ts
         - folder
            - source.ts
            - source.zod.ts
            - source.integration.ts

which works well.

Yes, it does sound hacky.
I feel this whole issue with handling any is hacky by nature...

Right now, I managed to get the tests to pass but not yet the yarn gen:example (we need to pass the right content to extraFiles from the cli.ts)

[tvillaren]

OK, the last version I pushed should cover the case of #207 (which I also added in the example provided through yarn run gen:example)

Right now, we're passing all the files from the ts-to-zod.config.js file to the validation, which is too much in most cases (as usually, not all files are imported in most cases). So this can be optimized.

Also, the CI passes even when there are errors in the test:ci step (if we had looked at the details of the last CI run for my initial PR adding imports, we would have seen the gen:example fail.

[schiller-manuel]

sidenote: exiting with code 1 on error might be a breaking change ... so we should increase to v4 maybe?

[schiller-manuel]

Full quote since there were messages in between:

Let's imagine you run ts-to-zod in /Users/me/ts-to-zod

This pwd becomes the root for the VFS. That's were the node_modules folder will be set, as well as the source / schema / integration files.

If you add ../zHero.ts as extra files with content

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:

- Users
   - me
      - zHero.ts
      - ts-to-zod
         - node_modules
         - source.ts
         - source.zod.ts
         - source.integration.ts

so the import of zod in zHero.ts doesn't work.

If we add extra folders, based on the maximum depth, we get:

- Users
   - me
      - ts-to-zod
         - node_modules
         - zHero.ts
         - folder
            - source.ts
            - source.zod.ts
            - source.integration.ts

which works well.

Yes, it does sound hacky. I feel this whole issue with handling any is hacky by nature...

Right now, I managed to get the tests to pass but not yet the yarn gen:example (we need to pass the right content to extraFiles from the cli.ts)

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?

[tvillaren]

sidenote: exiting with code 1 on error might be a breaking change ... so we should increase to v4 maybe?

Indeed, maybe we should change this in another PR, as this PR is meant to fix a bug :)
I'll revert

[tvillaren]

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?

You mean recreating the whole folder structure "above" the source & generated files from the original project in the VFS?
Yes, that could work as well.

This means we would need the "deepest" of both source & generated file path (as there might be relative imports in both)

[schiller-manuel]

Why do we need to "recreate" the existing folder / file structure at all?
Isn't the VFS like an overlay on top of the existing folders/files?

[tvillaren]

You're right, I was not too familiar with VFS upon writing. And we indeed use createFSBackedSystem which "hovers a virtual environment" (from the README)

But as the 3 files used for validation are currently put in the root folder of the VFS (as source.ts, source.zod.ts and source.integration.ts), we need to place source.ts and source.zod.ts at their "actual" location + compute related imports for source.integration.ts + make sure the extraFiles are placed at the right location as well.

[schiller-manuel]

let me know when it is ready to review please

[tvillaren]

@schiller-manuel: I believe it is ready for review 👍

[schiller-manuel]

does this slash need to take platform specifics into account ( e.g. maybe on Windows it should be a \)?

[tvillaren]

This is only meant to handle import paths, not absolute ones.

Can import paths have backslashes? I really don't know 🤷‍♂️

[schiller-manuel]

how about using path.sep instead of a hardcoded /?
see https://nodejs.org/api/path.html#pathsep

[schiller-manuel]

or even better, do not manually build the path, but instead use path.join:

return join(dir, name);

[tvillaren]

Done ✅

[schiller-manuel]

very cool, thanks a lot