Run & review this pull request in StackBlitz Codeflow.

🦋 Changeset detected

Latest commit: fb7b724

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

The latest updates on your projects. Learn more about Vercel for Git ↗︎

TODO

• Figure out why v.union is generated with newlines despite multiLine: false
• Work through previous integer generation code and decide how to change it (if at all) to work with the new integer format logic
• Look at failing test snapshots to decide if they can be updated or if further code changes are necessary
• Figure out how to integrate actual unit tests for generated validation behavior

@mrlubos I got it to work and have a first draft, but I have a few questions:

1. The createArrayLiteralExpression multiLine parameter is resolved as

   multiLine || (!Array.isArray(elements[0]) && typeof elements[0] === 'object')

   instead of just using the passed value. I assume it is intended and fine that v.union definitions generate like this:

   v.union([
     v.number(),
     v.string(),
     v.bigint()
   ])

   rather than as a one liner v.union([v.number(), v.string(), v.bigint()])?

2. In the Valibot plugin.ts numberTypeToValibotSchema function what is this code path for?

   if (typeof schema.const === 'number') {
     // TODO: parser - handle bigint constants
     const expression = compiler.callExpression({
       functionName: compiler.propertyAccessExpression({
         expression: identifiers.v,
         name: identifiers.schemas.literal,
       }),
       parameters: [compiler.ots.number(schema.const)],
     });
     return expression;
   }

   I am not clear on its purpose. If const in the schema means the property must equal that value, why generate code for it here and what does the TODO refer to?

3. Unrelated to this issue but an observation: in the 3.1.x spec schema-const.yaml test snapshot and schema-const/valibot.gen.ts, there is no literal check generated for bigint. Is that something I should address here or is it a separate bug? Could it relate to the TODO above? Also, is there a reason the other checks like min / max are generated with BigInt(123) instead of using the literal 123n?

4. I would like to implement a Vitest suite to validate the generated Valibot pipelines (for example to check that each scenario accepts and rejects the correct values). Where in the project structure would be the best place for those tests? Are there any examples already present I can follow in terms of code style, etc. ...?

Hey!

1. That looks fine to me. Sometimes it's a pain to get right with TypeScript Compiler API, but it might be a legitimate bug too. I wouldn't worry about it, it's not a big deal.
2. The current logic wouldn't correctly generate output for bigint values such as 123n, there's a similar TODO in the Zod plugin. const is a way in JSON Schema to restrict a value to a single value. If it's defined, we know no other value will be legal and we don't have to deal with other constraints – x is either equal to const or its validation should fail.
3. That's correct, it's related to point 2. If you handle it, that would be certainly appreciated! As for your last question, I'd need to see an example to understand, can't tell if there's a good reason for that.
4. I believe there are no existing examples. That's a noble aspiration which I fully support though, maybe openapi-ts-tests/test/<new_folder>? I imagine we'd want to do something similar for each plugin and these tests would be isolated per plugin, unlike snapshots?

Hey @mrlubos, thanks again for your feedback! A few follow-ups:

1. I just want to clarify this, because it is caused by the following code in openapi-ts/packages/openapi-ts/src/compiler/types.ts at lines 536–537. The createArrayLiteralExpression function has this logic:

const expression = ts.factory.createArrayLiteralExpression(
  elements
    .map((value) => (isTsNode(value) ? value : toExpression({ value })))
    .filter(isType<ts.Expression>),
  // multiline if array contains objects
  multiLine ||
    (!Array.isArray(elements[0]) && typeof elements[0] === 'object'),
);

The key issue is in the condition:

multiLine || (!Array.isArray(elements[0]) && typeof elements[0] === 'object')

This forces multiLine to be true if the first element in the array is an object. In my case, the array elements are TypeScript CallExpression nodes (like v.number(), v.string(), v.bigint()), which are JavaScript objects. Therefore typeof elements[0] === 'object' evaluates to true, overriding the explicit multiLine: false setting:

const unionExpression = compiler.callExpression({
  functionName: compiler.propertyAccessExpression({
    expression: identifiers.v,
    name: identifiers.schemas.union,
  }),
  parameters: [
    compiler.arrayLiteralExpression({
      elements: [
        compiler.callExpression({
          functionName: compiler.propertyAccessExpression({
            expression: identifiers.v,
            name: identifiers.schemas.number,
          }),
        }),
        compiler.callExpression({
          functionName: compiler.propertyAccessExpression({
            expression: identifiers.v,
            name: identifiers.schemas.string,
          }),
        }),
        compiler.callExpression({
          functionName: compiler.propertyAccessExpression({
            expression: identifiers.v,
            name: identifiers.schemas.bigint,
          }),
        }),
      ],
      multiLine: false,
    }),
  ],
});

This is why multiLine: false is ignored or overwritten. When I remove the second clause, everything is generated correctly (i.e., respecting the passed multiLine value). I wanted to clarify if this behavior is intended or might be an unintended side effect. If it is fine to leave it as is, I will not pursue it further.

2. Thank you for the explanation. I overlooked the typeof check in that if condition, and it makes perfect sense now. Regarding the TODO, I will try to handle it accordingly by supporting bigint constants (for example v.literal(123456789n)).

3. In JavaScript, you can create a BigInt either via BigInt(123456789) or by using the n literal suffix (123456789n). The literal suffix is like a shorthand for calling the constructor (not quite a shorthand, but rather built-in JS grammar). I used the n suffix for the generated min/max validations, but currently your code uses the BigInt constructor (as seen in the validator-bigint-min-max test snapshot). Excerpt:

export const vFoo = v.object({
  foo: v.optional(
    v.pipe(
      v.union([
        v.number(),
        v.string(),
        v.bigint()
      ]),
      v.transform(x => BigInt(x)),
      v.minValue(-9223372036854775808n, 'Invalid value: Expected int64 to be >= -2^63'),
      v.maxValue(9223372036854775807n, 'Invalid value: Expected int64 to be <= 2^63-1'),
      v.minValue(BigInt(0)),
      v.maxValue(BigInt(100))
    )
  )
});

You can see my logic for format validation, then the additional schema min/max limitation. I want to ask if one style is preferred over the other, if a mix is acceptable, or if we should standardize on one.

4. I think grouping tests by plugin makes sense. I will get started with some accept/reject Vitest cases and see where it takes me.

1. I say leave it. That compiler stuff should be refactored at some point. But if you're curious why, I think it's a leftover from when this function accepted only JavaScript objects. Later we probably added AST node support and that broke that condition. You can see above that line we explicitly check for node type.

2. I prefer BigInt for consistency. I'd expect a code formatter to swap these for the other syntax if they're truly equivalent and people prefer that syntax

Open in StackBlitz

npm i https://pkg.pr.new/hey-api/openapi-ts/@hey-api/nuxt@2344

npm i https://pkg.pr.new/hey-api/openapi-ts/@hey-api/openapi-ts@2344

npm i https://pkg.pr.new/hey-api/openapi-ts/@hey-api/vite-plugin@2344

commit: fb7b724

Codecov Report

Attention: Patch coverage is 27.15517% with 169 lines in your changes missing coverage. Please review.

Project coverage is 22.52%. Comparing base (08f219e) to head (fb7b724).
Report is 12 commits behind head on main.

@@            Coverage Diff             @@
##             main    #2344      +/-   ##
==========================================
+ Coverage   22.47%   22.52%   +0.05%     
==========================================
  Files         323      324       +1     
  Lines       31662    31857     +195     
  Branches     1232     1232              
==========================================
+ Hits         7115     7177      +62     
- Misses      24538    24671     +133     
  Partials        9        9              

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

@Daschi1 I moved tests around to accommodate both Zod 4 and Zod 3. Whatever was in packages/openapi-ts-tests is now in packages/openapi-ts-tests/main with the exception of specs which moved from packages/openapi-ts-tests/test/spec to packages/openapi-ts-tests/specs. Hopefully it's straightforward to pull the latest changes!

@Daschi1 I moved tests around to accommodate both Zod 4 and Zod 3. Whatever was in packages/openapi-ts-tests is now in packages/openapi-ts-tests/main with the exception of specs which moved from packages/openapi-ts-tests/test/spec to packages/openapi-ts-tests/specs. Hopefully it's straightforward to pull the latest changes!

I just finished implementing the tests, thank you for the heads up! I'll resolve the merge conflicts later today and let you know once it's ready for review.

@Daschi1 I moved tests around to accommodate both Zod 4 and Zod 3. Whatever was in packages/openapi-ts-tests is now in packages/openapi-ts-tests/main with the exception of specs which moved from packages/openapi-ts-tests/test/spec to packages/openapi-ts-tests/specs. Hopefully it's straightforward to pull the latest changes!

Hi @mrlubos,

I tried to integrate the recent changes over the last few hours, but honestly I struggled a lot, mainly due to my limited Git experience with large merges. I made some updates and now there are no merge conflicts with main, but I'm not sure if all my changes were applied correctly.

The PR now shows over 3,000 changed files because of the directory refactor, and I don't know if that is expected. Also, some tests that pull schemas are failing with 404 errors. I suspect it's because those files were moved, but I'm not sure if that is on my end or if the tests need adjustment.

I'm out of ideas and would really appreciate any guidance or assistance you can offer. I apologize if I overcomplicated this merge. My last option might be to close this PR and open a new one based on the latest code changes where I manually reimplement all my changes, so I can be sure everything is in the right place.

Thank you for your patience and any help you can provide!

Sorry for that! If resolving conflicts is a pain, I'd roll back your branch to commit aa5b7db1adab52b096dcad5ae2acef06bdf5b6ec so you get a cleaner diff.

From there, I'd start with a new branch based off main and move files over from your branch with git restore --source=<your-branch> <files>. That will give you a clean diff on top of the latest.

The only files you won't be able to move that way will probably be the changes to tests and specs. However, you should be able to re-apply those changes manually, I think it was just a few files.

Do NOT move snapshots over, they'll only be getting in the way. Instead, re-run pnpm openapi-ts-tests test:update once you've moved the source files over and built the library and that will give you the correct snapshots without causing merge conflicts.

You also added a commit feaf4d83b9c56448faf09a8ffefd13aa9d00c112 later, that one should be easily movable too

@Daschi1 CI tests are failing but your pull request might be okay. Stuff has moved around yesterday and I didn't fix the CI yet

@Daschi1 CI tests are failing but your pull request might be okay. Stuff has moved around yesterday and I didn't fix the CI yet

Yeah I noticed, some tests are failing because they attempt to download specs that no longer exist, which causes a 404. For example:

test/index.test.ts > index > downloads and parses v2 without issues
Generating from https://raw.githubusercontent.com/hey-api/openapi-ts/main/packages/openapi-ts-tests/test/spec/v2.json
Error: Request failed with status 404: Not Found

test/index.test.ts > index > downloads and parses v3 without issues
Generating from https://raw.githubusercontent.com/hey-api/openapi-ts/main/packages/openapi-ts-tests/test/spec/v3.json
Error: Request failed with status 404: Not Found

Sorry for that! If resolving conflicts is a pain, I'd roll back your branch to commit aa5b7db1adab52b096dcad5ae2acef06bdf5b6ec so you get a cleaner diff.

From there, I'd start with a new branch based off main and move files over from your branch with git restore --source=<your-branch> <files>. That will give you a clean diff on top of the latest.

The only files you won't be able to move that way will probably be the changes to tests and specs. However, you should be able to re-apply those changes manually, I think it was just a few files.

Do NOT move snapshots over, they'll only be getting in the way. Instead, re-run pnpm openapi-ts-tests test:update once you've moved the source files over and built the library and that will give you the correct snapshots without causing merge conflicts.

Thank you so much for that! That helped tremendously and the git restore command is definitely handy, I didn't know about it before. Everything should now be up to date with my changes, so I've marked the PR as ready for review. I am looking forward to your feedback, and if you have any questions or need adjustments I am happy to accommodate to get this merged :)

Looks good! There are a few remaining if conditions, can be merged after those are cleaned up

Oops, I had to undo some changes locally earlier and it looks like I was a bit too generous with my ctrl‑z :D
I pushed a fix now.