hey-api / openapi-ts

✨ Turn your OpenAPI specification into a beautiful TypeScript client
https://heyapi.vercel.app
Other
1.07k stars 89 forks source link

Generated angular client produces compile error due to tree shaking in services #638

Closed szacskesz closed 1 month ago

szacskesz commented 4 months ago

Description

The angular client generates a this.http reference in the "class-less" service files, but the this is always undefined because it is an arrow function, so it results in a compile error.

For example:

export const postAuthenticationAuthenticate = (data: PostAuthenticationAuthenticateData = {}): Observable<PostAuthenticationAuthenticateResponse> => { return __request(OpenAPI, this.http, {
    method: 'POST',
    url: '/Authentication/Authenticate',
    body: data.requestBody,
    mediaType: 'application/json-patch+json'
}); };

OpenAPI specification (optional)

No response

Configuration

npx @hey-api/openapi-ts -c angular -i .\api\swaggger\auth-swagger.json -o .\api\generated\auth\ --schemas false

System information (optional)

Windows 11, Node 18.20.3, NPM 10.7.0

mrlubos commented 4 months ago

Hey @szacskesz, there's a setting to get the previous default, tree-shaking is only enabled by default now but you can turn it off https://heyapi.vercel.app/openapi-ts/migrating.html#v0-46-0

szacskesz commented 4 months ago

Thanks for the fast response @mrlubos !

I know I can switch back to the previous behaviour, but the main reason I would like to use this library is the tree-shaking. So I will wait until it's fixed.

mrlubos commented 4 months ago

It might honestly take a while, Angular client isn't the priority at the moment!

jacobinu commented 1 month ago

I could help here, however, I am not familiar with Angular. Is the same HttpClient reused throughout the application? Would it be preferable to have as an input per service method or would we want to put it in the generated OpenAPIConfig?

szacskesz commented 1 month ago

Yes, the HttpClient should be a singleton instance. It could be a parameter (per service method), or we could replace the arrow functions with a normal functions so the binding of this is possible, also using the inject(...) function is a possibility. I dont know which is the best but I'd be happy with either.

jacobinu commented 1 month ago

Given that HttpClient is a singleton I think we would prefer to not pass the HttpClient in every service method.

I'm not sure how replacing the arrow function with the normal function would pickup the injection context. Sorry if I'm wrong, but it seems like the injection context can only be access by classes instantiated by the DI system (@Injectable/@Component). If you wanted to cut down on the unused service methods you could filter the endpoints.

The generated code exposes the OpenApiConfig in core/OpenAPI.ts

export const OpenAPI: OpenAPIConfig = {
    BASE: '/v3',
    CREDENTIALS: 'include',
    ENCODE_PATH: undefined,
    HEADERS: undefined,
    PASSWORD: undefined,
    TOKEN: undefined,
    USERNAME: undefined,
    VERSION: '1.0.20-SNAPSHOT',
    WITH_CREDENTIALS: false,
    interceptors: {
        response: new Interceptors(),
    },
};

I think adding the HttpClient here would be best so that the client is automatically accessed by all the service functions. How does that sound?

szacskesz commented 1 month ago

Using a normal function would make it possible to bind this, so at least the generated functions would compile, see:

@Component({...})
export class ExampleComponent {

    http = inject(HttpClient);

    user$ = getUser.bind(this)({
        requestBody: {
            userName: "Test",
        }
    });
}

But I agree it is not the perfect solution, only a temporary fix.

The OpenApiConfig sounds promising, we could add there the HttpClient instance there, but the question arises, how will it be set? The only way I see is setting it manually (for example in APP_INITIALIZER, a service or something like that).

If we could make larger changes my proposal would be:

Instead of the global OpenAPIConfig create an ApiService which holds the previous OpenAPIConfig config values, and has setters / getters for them. It should also have a request method which accepts a generated service method (for example getUser) and its parameters. This way we:

It could be used something like this:

import { Component, inject } from "@angular/core";
import { ApiService as UserApiService } from "src/api/generated/user/core/api.service";
import { getUser } from "src/api/generated/user/services.gen";

@Component({ ... })
export class ExampleComponent {

    private readonly userApiService = inject(UserApiService);

    // Here the requestBody and the response type comes from the `getUser` method
    user$ = this.userApiService.request(getUser, {
        requestBody: {
            userName: "Test",
        },
    });
}
jacobinu commented 1 month ago

I see that now OpenAPIConfig wasn't used before because the configuration happens in the HttpClient. Your proposal seems to be a part of a larger effort for better Angular support #667. With this in mind I think replacing the arrow function with the anonymous function is the best path forward as you suggested. I recommend using call instead of bind with this implementation.


user$ = getUser.call(this, {
        requestBody: {
            userName: "Test",
        }
});