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

[Project] Issue while adding more tests for validateGeneratedType #186

Closed
tvillaren opened this issue Dec 1, 2023 · 6 comments
Closed

Comments

@tvillaren
Copy link
Collaborator

tvillaren commented Dec 1, 2023

Hey @fabien0102,

I'm creating an issue for something which doesn't seem like a bug, at least wasn't able to reproduce in a "real-life" project but some of the tests I'm adding (for #135) now fail after a rebase on main (post-release of 3.4.1).

Context

A couple of month ago, I raised #140 because some validations were failing on my project due to a bug in zod.

I had similar validation fail after working on a built version of branch introducing import handling so decided to add some tests (74b10b3) to cover specific identifier references triggering optional any.
👉 Those tests don't pass anymore since I rebased the branch
👉 They still pass on another branch (https://github.com/tvillaren/ts-to-zod/tree/feat-handle-zod-imports) which I haven't rebased

I created a simple branch from the current main#HEAD with a simple not passing test (simple union in an object): https://github.com/tvillaren/ts-to-zod/actions/runs/7062596244/job/19226755896

I'm a bit lost, tried different things including fiddling with the fixOptionalAny file to add more edge cases but outputting the actual types from the vfs (acda151) gives me some discrepancies in type inference.

I'm starting to thing the Typescript inference doesn't work the same way in VFS that on my system (I checked version of TS passed and they are the same).
If you have any idea (or @schiller-manuel as you seem to have updated this project quite a lot recently 😉), I'm all hear!

(I'm running those on a Mac)

Thanks!

@schiller-manuel
Copy link
Collaborator

Hi @tvillaren,

I had a quick look at the simplified example and I saw that the schema is incorrect.
The parameter of z.union() should be an array:

diff --git a/src/core/validateGeneratedTypes.test.ts b/src/core/validateGeneratedTypes.test.ts
index 78fe5ea..bceb9c2 100644
--- a/src/core/validateGeneratedTypes.test.ts
+++ b/src/core/validateGeneratedTypes.test.ts
@@ -221,7 +221,7 @@ describe("validateGeneratedTypes", () => {
       import { z } from "zod";
   
       export const citizenSchema = z.object({
-          heroAliases: z.union(z.string(), z.number())
+          heroAliases: z.union([z.string(), z.number()])
       });
       `,
       relativePath: "source.zod.ts",

I did not check the other code, so it can be that the reproducer example was not representative for the real issue you are experiencing.
Can you check again?

@tvillaren
Copy link
Collaborator Author

tvillaren commented Dec 3, 2023

🤦
Yep, this typo fixes the use case...

So actually, the issue is different and only appears on my branch, which introduces import handling, when we reference imported types/interfaces (which are transformed to z.instanceof).
It is the same issue as discussed in colinhacks/zod#2203.

I spent some more time digging, and it appears the issue has nothing to do with my rebase to the last version of ts-to-zod: only related to colinhacks/zod#2203.

Trying to reproduce colinhacks/zod#2203 with validateGeneratedTypes

I implemented the use case described in colinhacks/zod#2203 in a new test (tvillaren@68acbed) and it passes validation with validateGeneratedTypes.
But when looking at the actual inferred type via a console.log output of VFS, I see the same behavior as mention in colinhacks/zod#2203:

type CitizenInferredType = {
  heroAliases: (string[] | number[]) & (string[] | number[] | undefined);
}

You can try comparing both type in your IDE:

// eslint-disable-next-line @typescript-eslint/no-unused-vars
function expectType<T>(_: T) {
  /* noop */
}

export type Citizen = {
  heroAliases: string[] | number[];
};

export const citizenSchema = z.object({
  heroAliases: z.union([z.string().array(), z.number().array()]),
});

export type CitizenInferredType = z.infer<typeof citizenSchema>;

expectType<CitizenInferredType>({} as Citizen);
expectType<Citizen>({} as CitizenInferredType);

(I tried it in a VS Code with same version of TS & zod without error).

I have errors though when using z.instanceof to handle import, and they probably come from the fact that the imported types are actually not in the VFS so they're replaced by any.

Anyway, sorry for the noise. This "issue" raised 2 questions for me:

  1. Seeing that validation works even when the inferred type is not really the initial one (see my example above), I'm not sure of its real value (it's not the first time I spend time on such issues 😅)
  2. Maybe using instanceof for 3rd-party imports is not completely relevant, as it does seem to work only for classes and not "simple types"

@fabien0102
Copy link
Owner

fabien0102 commented Dec 5, 2023

Yeah, expectType is not the most robust things, it's actually a known issue, the main idea of this validateGeneratedType is just to help me to catch the non-handled cases 😉 I think I saw better support for type assertion somewhere but never had the time to try

Maybe something to look at: https://www.totaltypescript.com/how-to-test-your-types

@tvillaren
Copy link
Collaborator Author

Yes, I was looking at alternatives. Found the type-testing library on Advent of Typescript

@schiller-manuel
Copy link
Collaborator

Isn't (string[] | number[]) & (string[] | number[] | undefined) reducing to string[] | number[] and thus expectType is not wrong here?

@tvillaren
Copy link
Collaborator Author

Closing, implemented some more cases to fixOptionalAny

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

No branches or pull requests

3 participants