-
-
Notifications
You must be signed in to change notification settings - Fork 206
Fixing widespread edge case of nullable dates in required fields in transformers #1917
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
Fixing widespread edge case of nullable dates in required fields in transformers #1917
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: d7af22c The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
@Freddis thank you for the pull request! Rest assured I read every comment. This package is still in v0 so expect to find some rough edges (as you successfully did) |
ea8f718
to
b10920d
Compare
@mrlubos thank you for your response, it's reassuring. Other than my issues with the plugin system, the library works just like a charm. It provides a solid out-of-the-box experience and I'm really enjoying using it at work and in my own projects. Looking at how tests went, I'm seeing that the problem with dates is not as bad as I've expected. There are 2 ways of how union types can be described in openapi:
or
For the latter case you have a test against, which is good. Unfortunately my fix conflicts with this test and creates unnecessary code duplication in resulting transformer for such case. I'll try to fix it tomorrow so you have both cases covered with clean output code. |
b10920d
to
38bd8a4
Compare
@hey-api/client-axios
@hey-api/client-fetch
@hey-api/client-next
@hey-api/client-nuxt
@hey-api/nuxt
@hey-api/openapi-ts
@hey-api/vite-plugin
commit: |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1917 +/- ##
==========================================
- Coverage 24.15% 24.14% -0.01%
==========================================
Files 193 193
Lines 26851 26861 +10
Branches 787 787
==========================================
Hits 6486 6486
- Misses 20290 20300 +10
Partials 75 75
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Looks good man, thank you!
Right now date transformer doesn't take into account nullable dates, which is quite a widespread case. It produces this code:
I suppose everybody is experienced enough to see it's gonna give your consumers a data corruption in no time.
The PR doesn't account for all possible cases, only for Date | null.
I'm not sure if the author of the repo is reading through PRs, but if you do, I tell you what:
The plugin system right now is in absolutely disastrous state. You won't get much help by outright lying in your docs.
We are not in the same boat! Your built-in plugins do communicate with each other somehow and they have access to modules that are not exported from the project.
The only reason I'm sending this PR is that I don't want to have your project hanging in my repo with source files. Under different circumstances I would never sent a PR after spending 6 hours on a 3 minute fix because I believed you that we are in the same boat. We clearly are not.
I can't imagine how many others tried to write their own plugins only to find out you can't do anything with them. To replicate the behavior of transformers you need to resort to copying and pasting files from original code and in the end SDK plugin simply ignores yours, because yours is not "@hey-api/transformers".
Decouple your plugins and the project is going to bloom. For now, you can at least allow to set the same names as built-in plugins for your own plugins to create an override, so we could copy / paste the code and fix problems there or tweak the behaviours.
EDIT AFTER FINALIZING FIX:
Turned out that the original transformers for nullables were fine in what schemas they supposed to pick up. The problem was in discriminating against required fields in object schemas. Required means that value is present, but doesn't guarantee that the value is not nulish.