-
-
Notifications
You must be signed in to change notification settings - Fork 202
Typeid support #2034
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
Typeid support #2034
Conversation
|
🦋 Changeset detectedLatest commit: 257dd07 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 |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
commit: |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2034 +/- ##
==========================================
- Coverage 23.89% 23.81% -0.09%
==========================================
Files 317 317
Lines 29330 29450 +120
Branches 1229 1230 +1
==========================================
+ Hits 7009 7014 +5
- Misses 22312 22427 +115
Partials 9 9
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:
|
Hey @mrlubos before adding tests and solving the merge conflicts I was wondering what your thoughts were on the previously raised questions in the initial PR message? Or alternatively should openapi-ts get a better plugin interface so this can be done externally? |
Hi @Le0Developer, are you able to keep using the preview builds generated by this pull request? I will have some thoughts around developer experience but I think most of your ideas/questions are valid and need addressing, including a better plugin interface. Unfortunately, the current priorities lie elsewhere so I won't have time to address this improvement just yet, but wanted to give you at least some response |
@Le0Developer are you still interested in this feature? |
Yes! We are currently running the current state in prod and haven't had the opportunity to upgrade and deal with the other breaking changes (fetch client moving etc) |
31bdccd
to
ea6192a
Compare
@Le0Developer do we even need a separate configuration option for the |
I've currently decided to do it like this because of the This is semi-related to my 2nd question:
|
For me it comes back to the question: would users ever end up with As for your state question, have a look at how it's handled in TanStack Query
You could create a similar RE exporting, I don't see any harm in exporting |
ea6192a
to
875aaf6
Compare
Removed the option and added state tracking. I was hoping there'd be a nicer way than passing it around everywhere (eg on the plugin object or something), but alas. Also exported the TypeID type. Now that we have state tracking, number 3 is possible:
We currently have typeid "redefinitions" (eg a bunch of Q: since this needs snake_case to CamelCase conversion, should I reuse the |
875aaf6
to
10b034d
Compare
Implemented. This is how it's looking now: diff --git a/examples/openapi-ts-fetch/src/client/types.gen.ts b/examples/openapi-ts-fetch/src/client/types.gen.ts
index 6d8a6e7b..e3ecbeef 100644
--- a/examples/openapi-ts-fetch/src/client/types.gen.ts
+++ b/examples/openapi-ts-fetch/src/client/types.gen.ts
@@ -2,7 +2,7 @@
export type Order = {
complete?: boolean;
- id?: number;
+ id?: OrderId;
petId?: number;
quantity?: number;
shipDate?: string;
@@ -694,6 +694,10 @@ export type UpdateUserResponses = {
200: unknown;
};
+export type TypeID<T extends string> = `${T}_${string}`;
+
+export type OrderId = TypeID<'order'>;
+
export type ClientOptions = {
baseUrl: 'https://petstore3.swagger.io/api/v3' | (string & {});
}; From my perspective this is in a good state now (except that we parse the name from the |
Me too. We can explore it later if you want, this is usually faster and works.
I'm not sure I understand what you're referring to!
Think you already did this, looks good! Tests are the only remaining thing, do you need any guidance? |
Yes please, I don't see any obvious place for tests of the typescript "plugin" and can't find other tests. |
10b034d
to
f324e9e
Compare
There are three |
For your failing CI, run |
Yeah there's a few places where state isn't being passed as expected, I'm working on it. I'd love proper typeid support (with |
yeah I think we're a long way out from that. Happy to merge this once you got the tests and CI passing. Would love to know how much setup it's on the backend side to get the OpenAPI integration working? |
Do you mean in general? We use typeid-go and SQLc for our database wrapper and native types in the database ( We tell SQLc to overwrite the types like this: overrides:
- db_type: user_id
go_type:
type: UserID And then have a struct like this: type UserPrefix struct{}
func (UserPrefix) Prefix() string { return "user" }
type UserID struct {
typeid.TypeID[UserPrefix]
} SQLc will then use the custom type for all database queries. For user input we call So our models contain our custom UserID struct which is then picked up by swag when generating our OpenAPI docs. |
@Le0Developer do you want to connect on Discord or somewhere else? I could probably use your help with Go |
Feel free to add me |
f324e9e
to
dc515d4
Compare
dc515d4
to
37adcc7
Compare
See #2033
This is a very rough draft. I originally tried to use the
x-typeid-type
property I proposed in jetify-com/typeid#45. Then I noticed there's an IR involved and decided to temporarily extract the type name from theexample
field instead (which I also had to add to the parsers/IR).I've tested this by modifying the
openapi-ts-fetch
example. I downloaded the openapi.yml file and modified theid
property ofOrder
to be a typeid like this:And added
typeids: true,
to theopenapi-ts.config.ts
file.This produced this output:
Nice 🎉
Currently the TypeID declaration is always added. Which means if you enable the config option but don't use typeids, you'll get an unused type warning.Questions
x-typeid-type
instead of parsing theexample
value, but I'm unsure given the current IR infrastructure. I don't believe adding arbitrary extensions to the parser is maintainable. One I had was: what if the IR had an extra field which simply contained the original object or justx-*
properties? This feels more future-proof to me.ShouldTypeID<T>
be exported or should consumers doUser["id"]
instead ofTypeID<"user">
if they want to access the type themselves?Should there be a concrete type per ID (egtype UserID = "user_${string}"
) instead of theTypeID
"overlord" (or maybe both)? This would require extending the "State" interface to keep track of already generated ID types