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

Breaking Change #81

Closed
samchungy opened this issue Dec 20, 2022 · 5 comments · Fixed by #82
Closed

Breaking Change #81

samchungy opened this issue Dec 20, 2022 · 5 comments · Fixed by #82

Comments

@samchungy
Copy link
Contributor

Oops I accidentally shipped a breaking change, though Zod shipped the breaking change first 🤔

https://github.com/asteasolutions/zod-to-openapi/pull/78/files#diff-d8192a426c76954f6ca5af42fd2975758ff8e25940d33c3959c5317b4dabfb77R24

What should we do about it. If a consumer has lib check enabled and pulls the latest version of this library with an older version of Zod it'll complain.

@AGalabov
Copy link
Collaborator

Ohhh so the problem is with the two anys vs three anys? And now if the peer dependency of zod is of older version, then the type definition would not suffice as it would require a third parameter?

Do you happen to know if having the code in the state before your PR (i.e having 3 anys passed) would work with a newer version of zod. I think it would still break, right? Since we are passing an additional type parameter.

And if that is the case I guess we have only one valid option considering how reliant we are on zod. And that is to just bump up the zod peer dependency version. That would fix everything - we might need to release 4.0.0 but c'est la vie.

However if passing 3 anys to newer zod does not break anything then I guess we can bring down the dev dependency, restore the 3 anys and just keep your feature for datetime using some casting in order to obtain isDatetime. And we can add a TODO + an issue to upgrade to zod at a further point in time (when it actually makes sense feature wise to bump the peer dependency).

Do you have an answer/opinion on those two approaches?

@Naktibalda
Copy link

I had code with 3 any and it was broken by zod upgrade.

@Naktibalda
Copy link

See my comment at colinhacks/zod#1290 (comment)

@samchungy
Copy link
Contributor Author

samchungy commented Dec 20, 2022

Ohhh so the problem is with the two anys vs three anys? And now if the peer dependency of zod is of older version, then the type definition would not suffice as it would require a third parameter?

Do you happen to know if having the code in the state before your PR (i.e having 3 anys passed) would work with a newer version of zod. I think it would still break, right? Since we are passing an additional type parameter.

And if that is the case I guess we have only one valid option considering how reliant we are on zod. And that is to just bump up the zod peer dependency version. That would fix everything - we might need to release 4.0.0 but c'est la vie.

However if passing 3 anys to newer zod does not break anything then I guess we can bring down the dev dependency, restore the 3 anys and just keep your feature for datetime using some casting in order to obtain isDatetime. And we can add a TODO + an issue to upgrade to zod at a further point in time (when it actually makes sense feature wise to bump the peer dependency).

Do you have an answer/opinion on those two approaches?

The new version of Zod also breaks with our old version so I guess we're kinda forced to do a breaking change anyway I guess. Stuck between a rock and a hard place 😆

@AGalabov
Copy link
Collaborator

AGalabov commented Dec 20, 2022

@Naktibalda @samchungy thanks for confirming. I guessed so as well. Well then we'll do it. @samchungy do you have the capacity to open up a PR right now or should I do it in about an hour from now?

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 a pull request may close this issue.

3 participants