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

Add import handling for external classes #135

Merged
merged 25 commits into from
Feb 5, 2024

Conversation

tvillaren
Copy link
Collaborator

@tvillaren tvillaren commented May 23, 2023

Why

This PR adds support for imports.

The following input:

// source.ts
import { Person } from "@3rdparty/person";

export interface Hero {
  name: string;
  realPerson: Person;
}

will output

//output.ts

const personSchema = z.any();

export const heroSchema = z.object({
  name: z.string(),
  realPerson: personSchema,
});

@codecov-commenter
Copy link

codecov-commenter commented May 24, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (4f4e68e) 96.90% compared to head (fa6bfcc) 97.05%.

Files Patch % Lines
src/core/generateZodSchema.ts 75.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #135      +/-   ##
==========================================
+ Coverage   96.90%   97.05%   +0.15%     
==========================================
  Files          14       15       +1     
  Lines         742      782      +40     
  Branches      309      316       +7     
==========================================
+ Hits          719      759      +40     
  Misses         23       23              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tvillaren
Copy link
Collaborator Author

Blocked by #140

@tvillaren tvillaren changed the title [WIP] Add import handling for external classes Add import handling for external classes Jun 28, 2023
@tvillaren
Copy link
Collaborator Author

Hey @fabien0102,

Thank you for your recent update & merge.
Following those, here is proposal for a feature addition to handle non-relative imports. I already use it today (the only missing bit to ship it was the validation issue #140).

I acknowledge that this feature is a bit opinionated as it takes any non-relative import as not being a zod schema.
Happy to exchange about it here.

As a follow-up, I see an opening to handle relative imports as zod schemas (generated or not) and may open up the door to multi-file generation support eventually.

@dan-blank
Copy link

dan-blank commented Jul 6, 2023

Thanks for the PR!

I acknowledge that this feature is a bit opinionated as it takes any non-relative import as not being a zod schema. Happy to exchange about it here.

I think this is a sensible default. One option came to my mind:

Use a comment to tell ts-to-zod that the import is assumed to be a zod schema.

// source.ts

import { Person } from "@3rdparty/person";

// ts-to-zod ImportIsZodSchema Person

export interface Hero {
  name: string;
  realPerson: Person;
}

Its semantics would be:

  • Once we parsed // ts-to-zod ImportsIsZodSchema Person, we assume at every use site of Person that Person is a zod schema
  • Does not work for * in imports. When encountering *, log an error ("Only works for explicit imports") and use the default (= assume that the import is not a zod schema)
  • Does not work for as in imports. Also log and default.

It works for non-relative modules only, based on the principle that we might want to handle relative modules using the ts-to-zod conversion.

I don't understand the reasoning, could you explain/clarify in your OP? :)

@tvillaren
Copy link
Collaborator Author

The rationale behind using only non-relative imports for this feature was mostly based on the rationale that 3rd party imports had very little chance to be Zod Schema.

Relative imports, on the other hand, have maybe a higher chance to be a class / type / interface that is also transformed somewhere in the codebase into a zodSchema.
👉 For those, there are different handling strategies (like using JSTags-style comments) to tell the transpiler we have a zodSchema for this somewhere
👉 And I had as a future step to maybe fiddle with multiple file ts-to-zod (that would be based on relative imports obviously)

👉 In the meantime, it's quite easy to handle any imports as 3rd party ones and have specific JSTags comments in the future to handle zod schema (generated or referenced manually)

@tvillaren
Copy link
Collaborator Author

Hello @dan-blank, @fabien0102 👋

I updated this PR to allow any type of imports (non-relative and relative modules alltogether).
For now, it assumes that imports are to be used as z.instanceof(), I'm going to work on another PR to add JSTags for import statement to allow specifying when we reference a zod schema.

WDYT?

@anthony-dandrea
Copy link

Looks good here as well! Ran into the same small issue mentioned in your other PR.

@tvillaren
Copy link
Collaborator Author

Hello @fabien0102 , @schiller-manuel

After much discussions and thoughts, I reworked this Pull Request to support imports and generate a z.any() for all type references coming from an import.

It was previously using z.instanceof(ImportName) but it was an overlook from my side of the official documentation where instanceof is meant to be used only for imported Class (not types or interfaces).

Additionally, I had to update the fixOptionalAny bit that was added a couple of months ago to support type references as well.

But now this supports simply the imports, and I will rebase #148 to support imports of Zod Schemas as well.

Copy link
Collaborator

@schiller-manuel schiller-manuel left a comment

Choose a reason for hiding this comment

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

looks good, would love to get this and #148 merged asap.
can you clean this up (see my comments) please?

src/core/generate.ts Outdated Show resolved Hide resolved
src/core/generate.ts Outdated Show resolved Hide resolved
@tvillaren
Copy link
Collaborator Author

Hey @schiller-manuel,

Thank you for you review, I just cleaned the two commented lines.
I'm working on #148 and will push an update ASAP.

Thanks.

@schiller-manuel schiller-manuel merged commit a8ea597 into fabien0102:main Feb 5, 2024
4 checks passed
@tvillaren tvillaren deleted the feat-handle-imports branch February 6, 2024 20:32
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.

5 participants