-
Notifications
You must be signed in to change notification settings - Fork 0
Feature implementation from commits 89b41b5..494583f #4
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
base: feature-base-branch-2
Are you sure you want to change the base?
Conversation
according to SEMVER spec, this must be a minor update: > Minor version Y (x.Y.z | x > 0) MUST be incremented if new, backward compatible functionality is introduced to the public API.
@@ -65,6 +65,10 @@ export const getZodiosEndpointDefinitionList = (doc: OpenAPIObject, options?: Te | |||
.otherwise((fn) => fn); | |||
|
|||
const ctx: ConversionTypeContext = { resolver, zodSchemaByName: {}, schemaByName: {} }; | |||
if (options?.exportAllNamedSchemas) { | |||
ctx.schemasByName = {}; |
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.
🐛 Correctness Issue
Property name mismatch with type definition.
The code initializes ctx.schemasByName but the type definition uses schemaByName (without 's'), which will cause runtime errors when accessing this property.
Current Code (Diff):
- ctx.schemasByName = {};
+ ctx.schemaByName = {};
📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
ctx.schemasByName = {}; | |
ctx.schemaByName = {}; |
🔄 Dependencies Affected
lib/src/template-context.ts
Function: any function using getZodiosEndpointDefinitionList result
Issue: Code expecting schemaByName property but receiving schemasByName
Suggestion: Update any code using ctx.schemasByName to use ctx.schemaByName instead
dependencies.add(schemaName); | ||
// Sometimes the schema includes a chain that should be removed from the dependency | ||
const [normalizedSchemaName] = schemaName.split("."); | ||
dependencies.add(normalizedSchemaName!); |
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.
🐛 Correctness Issue
Non-null Assertion on Potentially Undefined Value.
The non-null assertion operator on normalizedSchemaName could cause runtime errors if the split pattern produces unexpected results.
Current Code (Diff):
- dependencies.add(normalizedSchemaName!);
+ dependencies.add(normalizedSchemaName || schemaName);
📝 Committable suggestion
‼️ IMPORTANT
Trust, but verify! 🕵️ Please review this suggestion with the care of a code archaeologist - check that it perfectly replaces the highlighted code, preserves all lines, maintains proper indentation, and won't break anything in production. Your future self will thank you! 🚀
dependencies.add(normalizedSchemaName!); | |
dependencies.add(normalizedSchemaName || schemaName); |
PR Summary
Schema Refinement Feature and Schema Handling Improvements
Overview
This PR adds schema refinement capabilities, improves schema name handling for duplicates, and fixes issues with falsy default values and string trimming.
Change Types
Affected Modules
src/openApiToZod.ts
src/template-context.ts
src/getZodiosEndpointDefinitionList.ts
tests/*
Notes for Reviewers