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

Start using components with $ref #130

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

A5rocks
Copy link
Contributor

@A5rocks A5rocks commented Jun 5, 2023

This change as far as I can tell is backwards compatible due to the overloads, though it does add a deprecated case.

I have not tested this at all so it probably doesn't work. Just making this to signal intent and solicit early feedback. Fixes #108.

EDIT: tests are now made. I think that showcases what this PR does. Pretty sure it's not exhaustive but not sure what other tests you might want!

@nx-cloud
Copy link

nx-cloud bot commented Jun 5, 2023

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 75c66b2. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 2 targets

Sent with 💌 from NxCloud.

@Brian-McBride
Copy link
Contributor

I pulled in another PR that updates the underlying open API schema lib. I think you'll need to update this PR as well.

@A5rocks A5rocks force-pushed the start-using-components branch from 912d0cf to 984541d Compare June 16, 2023 10:52
@A5rocks
Copy link
Contributor Author

A5rocks commented Jun 16, 2023

@Brian-McBride hi! So, everything's typed as OpenAPI 3.1 schemas meaning e.g. no "nullable" field. I actually have a change locally that in theory updates everything and was about to push it when I realized that's probably a bit too much of a change (and I've got one test I can't really figure out, plus it somehow breaks a feature...).

Basically, should I:

  • push a ton of changes to use openapi 3.1 as the types indicate
  • switch the types to openapi 3.0 (and then keep those changes to make a proper PR that can be in a minor or major release).

@A5rocks A5rocks force-pushed the start-using-components branch from 6f59543 to 984541d Compare June 16, 2023 11:37
@A5rocks
Copy link
Contributor Author

A5rocks commented Jun 16, 2023

I'll just change back types to openapi 3.0 for now...

@Brian-McBride
Copy link
Contributor

I am sorry, I really need a co-maintainer.

It looks fine. RIght now, everything is at 3.1 right?
The concern is the tests that go from exclusiveMinimum: 0 to exclusiveMinimum: true

I'll pay attention to the repo close here for the next few days. I'd suggest we bring it to he "current" standards. I just did a major release because the peer dep is a major release. We could do a another major?

But again, a test that goes from 0 to true seems off. I could see 0 going to false and not breaking downstream code.

@A5rocks
Copy link
Contributor Author

A5rocks commented Jun 30, 2023

It looks fine. RIght now, everything is at 3.1 right?

Err, no. At the moment, main branch is a strange combination of 3.0 and 3.1. For instance, it still uses nullable. I had a decently large diff to make everything match 3.1 behavior but I decided it would probably be best out of this PR.

I think I have that stashed somewhere, if you want it as another (or in this?) PR.

@DHFW
Copy link

DHFW commented Oct 31, 2023

Hello @A5rocks @Brian-McBride, what is the status of this? I would really appreciate if this goes to main. Thank you!

@A5rocks
Copy link
Contributor Author

A5rocks commented Oct 31, 2023

Sorry about the wait; I'll see if I can find my stash that moves things to 3.1 and make that a separate PR (after fixing it up and bringing it up to date)

@DHFW
Copy link

DHFW commented Nov 22, 2023

@A5rocks sorry man, did you had the time to work on this? Looking forward to it. Thanks!

@A5rocks
Copy link
Contributor Author

A5rocks commented Nov 23, 2023

Sorry, I put this off then forgot. I've now made #173 for that OpenAPI 3.1 support.

Thank you for reminding me!

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.

[zod-nestjs][zod-openapi] use $ref instead of properties for items
3 participants