Skip to content

Conversation

@luqasn
Copy link
Contributor

@luqasn luqasn commented Oct 22, 2025

First of all: awesome library! As proponents of type-safe, schema-first development, we couldn't find any alternative to your great project that could provide us with that. Thanks!

We were trying to use an OpenAPI feature that was not supported by the openapi-typescript version this project originally used (#refs to other files). So we bumped "all the things" and fixed what needed fixing.

One of the things that we struggled with as non-guru-level node developers was invoking the cli because of all the some package.json setup with module and what-not. So we split up the project into several packages, which we liked anyways from an architectural perspective.

Note that this means you'd have to create a new npm package for the cli itself and the README would need to be adjusted for the codegen part if you decide to keep this.

If you want, I can adjust the README if you like the change and want to keep it.

Supersedes #9

@luqasn luqasn mentioned this pull request Oct 22, 2025
@luqasn
Copy link
Contributor Author

luqasn commented Oct 22, 2025

@alabs-tomscholz in the linked PR you were saying something about remaining bugs, which ones are that?

@tomscholz
Copy link

tomscholz commented Oct 22, 2025

The one I can think of, I fixed in 1d01b4b

@luqasn
Copy link
Contributor Author

luqasn commented Nov 1, 2025

@henhal could you take a look? We'd love to go back to upstream from our fork.


export abstract class ApiError extends Error {
protected constructor(readonly request: RawRequest, message?: string) {
super(message);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this line removed? It's simply so that sub-classes of this error are named properly, causing logs to show e.g. BadRequestError: some message instead of Error: some message

Copy link
Contributor Author

@luqasn luqasn Nov 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was my understanding that newer javascript targets (ES6 and up) no longer need this.
And we rely on a newer feature anyways here (the cause for wrapping exceptions), so I think there is no gain in having this backwards compatible.
But open to change any of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked my expectations... and they were wrong. Logging the error name indeed spelled Error. So I put the line back in. I guess we can still only get rid of half of the hacks (the prototype chain) even in modern JavaScript, which baffles me.

@henhal
Copy link
Owner

henhal commented Nov 7, 2025

Hi @luqasn

Again, sorry for the slow response here. That daytime job just keeps coming in between. :)
Anyway, great work! I don't have a lot of time thoroughly testing this in detail, but I've decided to trust you on this one, splitting this up seems like a good idea, but honestly I've never worked with mono-repo workspaces before. Would be great if you could also update the README as mentioned.

The very first version of this library was openapi-ts-backend, but it was later split into @openapi-ts/typegen, @openapi-ts/backend and @openapi-ts/request-types. And now, I guess @openapi-ts/cli is also being extracted to a separate package?

@luqasn
Copy link
Contributor Author

luqasn commented Nov 8, 2025

No worries, life can be busy.
I now also updated the README with the new package name. There is no CI setup for publishing that I need to take care of, right?

@luqasn
Copy link
Contributor Author

luqasn commented Dec 4, 2025

@henhal are you still interested in the changes?

@henhal henhal merged commit de9353a into henhal:main Dec 5, 2025
@henhal
Copy link
Owner

henhal commented Dec 5, 2025

Again, sorry for being a 🐌 .
Are the other three PRs still applicable? I guess they need to be rebased from main now that this is merged.
Does it make sense to make a 3.0 release now or do you intend to update these PRs and have them merged as well before doing that?

Also: no CI set up for this (yet, at least)

@luqasn
Copy link
Contributor Author

luqasn commented Dec 5, 2025

Hey, great to see you :)
The other PRs are still applicable (I think they are great, but you decide for yourself :D), so I rebased all of them now.

#12 is awaiting upstream review and release, so I'd hold on that one.

#13 can go in no problem, very minor additional option.

#11 is a breaking change (handler return value shape changed). It can also go into the next release, but I'd do a 4.0 then.

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.

4 participants