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

Improvements for external plugins #1443

Closed
tompuric opened this issue Dec 17, 2024 · 10 comments
Closed

Improvements for external plugins #1443

tompuric opened this issue Dec 17, 2024 · 10 comments
Labels
feature 🚀 New feature or request

Comments

@tompuric
Copy link

Description

Hi team,

I've written my own plugin and it does the job!
I have followed the general guide here https://heyapi.dev/openapi-ts/plugins
But I came across some friction in the process that would be great to have resolved.

Let me know if there's any additional information you require. Thanks

🎟️ Problem 1. Generated barrel file does not include my plugin

If I have a plugin, I expect that plugin to be added to the index.ts barrel file.

Atm it produces

// This file is auto-generated by @hey-api/openapi-ts
export * from './types.gen';
export * from './sdk.gen';

When it should produce

// This file is auto-generated by @hey-api/openapi-ts
export * from './types.gen';
export * from './sdk.gen';
export * from './my-custom.gen';

🎟️ Problem 2. Unable to have my own typed Plugin.UserConfig

The current type system does not allow for external plugin types to be used. The end result is that I need to cast my Plugin type to any before using it.

const myCustomPlugin: Plugin.UserConfig<any> = defineConfig({
  name: 'my-custom-plugin',
});

🎟️ Problem 3. Unable to import IR types

I had to do some heavy typescript extraction in order to get IRContext, IROperationObject, IRParameterObject, and IRResponseObject. I use these for utility functions to map data to output. It would be beneficial to have these types (any potentially others) exposed.

ℹ️ Additional Context

My plugin index.ts

import type { Plugin } from '@hey-api/openapi-ts';

import { handler } from './plugin';
import type { Config } from './types';

export const defaultConfig: Plugin.Config<Config> = {
  _dependencies: ['@hey-api/typescript'],
  _handler: handler,
  _handlerLegacy: () => ({}),
  name: 'my-custom-plugin',
  output: 'my-custom-plugin',
};

/**
 * Type helper for `my-custom-plugin` plugin, returns {@link Plugin.Config} object
 */
export const defineConfig: Plugin.DefineConfig<Config> = (config) => ({
  ...defaultConfig,
  ...config,
});

My plugin types.d.ts

import type { Plugin as OpenApiPlugin } from '@hey-api/openapi-ts';

export interface Config extends BaseConfig {
  /**
   * Plugin name. Must be unique.
   */
  name: 'my-custom-plugin';
  /**
   * Name of the generated file.
   *
   * @default 'my-custom-plugin'
   */
  output?: string;
}
@tompuric tompuric added the feature 🚀 New feature or request label Dec 17, 2024
@mrlubos
Copy link
Member

mrlubos commented Dec 17, 2024

Hi @tompuric, congratulations on creating your own plugin! You're the first known instance haha. I know other people used it too, but I didn't get any feedback on how it went.

Are you able to provide more context around what your plugin does?

As for your pain points:

  1. I suppose this depends on the usage, hence the question above. Besides that, how would you resolve a conflict if multiple files export the same type or function?
  2. Why do you need to assign an explicit type to the variable? Doesn't the returned type from defineConfig() give you the correct type?
  3. That makes sense. Any preference on exporting these as a namespace vs named exports?

I've also noticed you're aliasing the import in your types.d.ts snippet. Why?

Thank you for using Hey API!

@josstn
Copy link

josstn commented Dec 18, 2024

Re: point number 3, I'm currently facing similar challenges. Trying to create my own "methodNameBuilder", I would like to use stringCase and sanitize util functions, but cant find them exported anywhere, so I guess I have to copy them out from the source.

Would It be a solution to perhaps export internal types and util functions from one or more "submodules" instead of adding everything to the index barrel file? So, adding something like

"./utils/*": {
  "import": "./src/utils/*"
}

to the exports clause in package.json?

Then plugin developers, etc, (using esm module imports) can import directly from the file, and the base module is not polluted with "everything".

@mrlubos
Copy link
Member

mrlubos commented Dec 18, 2024

@josstn please don't do that 😂 I mean it's fine if you need it asap, but I appreciate the feedback and this is something we can expose and quickly, so might save you some time there.

I thought about submodules, but it's not clear to me at this point what else I might want to expose. Also, if I expose everything, people will inadvertently start using it and changing it will become painful. So being intentional about what's exposed makes more sense to me. I think it's also nice when you can use autocompletion to see what else is available instead of digging through submodules, but this might be a personal preference.

tl;dr let me expose more stuff for you

@tompuric
Copy link
Author

tompuric commented Dec 18, 2024

Are you able to provide more context around what your plugin does?

We are converting from one generator to another. The models produced are fine but the sdk is incompatible with ours. So, as a migration strategy, to mitigate large sweeping changes, I needed to override the hey-api's sdk and thus created my own plugin to replicate our prior generators sdk. This custom plugin is meant to be temporary while we at some future date look into moving to the libraries default sdk. Something to note here is that I heavily relied on string template literals to get the job done as utilising typescript as your library does was a bit heavy duty for my needs. I would have used it if your utility functions we're provided however. They look quite handy, I thought about copying them across whilst working on this plugin.

how would you resolve a conflict if multiple files export the same type or function

My assumption here is that it's the plugins responsibility to avoid conflicts. In our case, we just don't produce the default sdk plugin. I also just checked whether if both sdk's would have conflicts and it does not. It's odd to me that internal and custom plugins are treated differently. Unless I've just misconfigured my plugin and there's an option to allow that behaviour somewhere?

Why do you need to assign an explicit type to the variable? Doesn't the returned type from defineConfig() give you the correct type?

The returned type from defineConfig is correct. The issue is the plugins types from createClient. It does not accept any config that isn't already defined within the repo.

See type: plugins?: ReadonlyArray<UserPlugins['name'] | UserPlugins>;

This is the type error I get.

Type 'Config<Config>' is not assignable to type '"@hey-api/schemas" | "@hey-api/sdk" | "@hey-api/transformers" | "@hey-api/typescript" | "@tanstack/angular-query-experimental" | "@tanstack/react-query" | "@tanstack/solid-query" | ... 4 more ... | UserPlugins'.
  Type 'Config<Config>' is not assignable to type 'UserConfig<Config$b> | UserConfig<Config$a> | UserConfig<Config$9> | UserConfig<Config$8> | ... 6 more ... | UserConfig<...>'.
    Type 'Config<Config>' is not assignable to type 'UserConfig<Config$1>'.
      Types of property 'name' are incompatible.
        Type '"my-custom-plugin"' is not assignable to type '"zod"'.

Any preference on exporting these as a namespace vs named exports?

No preference. So long as I have access to the types somehow, i'd be happy :)

Appreciate the responses <3

@mrlubos
Copy link
Member

mrlubos commented Dec 18, 2024

@tompuric is that your custom codegen that you're migrating from? If it's another popular package, we could add it as a plugin to help other people

Conflicts - only SDK and TypeScript are treated differently. I'm open to making it configurable, but we need to resolve how to deal with conflicts. What if your plugin exports a type Foo but there's also Foo in types.gen.ts? Or your plugin exports a function foo() but so does sdk.gen.ts?

Types - ah yeah, I think that's one part that I left unfinished. This whole custom plugin interface was put together quite quickly in response to someone asking for it. Definitely room to improve!

@tompuric
Copy link
Author

Apologies @mrlubos, it's not publicly available as it was a custom built one that's specific to our tooling. I wouldn't recommend it ;)
It was forked from https://github.com/acacode/swagger-typescript-api if that helps.

As for the conflicts discussion. Our custom plugin does not export any types, as we leverage off your types. As for the sdk function names, we have our own methodNameBuilder. So there's no conflict for us. I see your concern here. Just providing my experience and thoughts on the matter.

@mrlubos
Copy link
Member

mrlubos commented Dec 18, 2024

Okay, I'll expose an API for exports, with the caveat that you'll have to know what you're doing for now. Guarantee someone will break it soon though 😂

RE migration, how much customisation did you add to your fork?

@tompuric
Copy link
Author

how much customisation did you add to your fork?

Oh boy... though it might be helpful if it helps guides future direction 🤷

I'd say these are the main differences. The remaining differences are internals that lead towards those input/output types

  • Our operation return types use resultful (yours uses RequestResult)

  • Our operation return types are explicit with which statuses return which models (yours merges these types into Success/Failure unions)

export type ModelResponseType =
  | Readonly<{ body: DataResponse; status: 200; response: Omit<Response, 'status'> & { status: 200 } }>
  | Readonly<{ body: ErrorResponse; status: 400; response: Omit<Response, 'status'> & { status: 400 } }>
  ...
  • Our operation parameters accept baseUrl + "hooks" (yours solves this via a custom client, or client configuration, and client interceptors)

@mrlubos
Copy link
Member

mrlubos commented Dec 19, 2024

@tompuric I'm not sure if you're following, but you'll be able to force your plugin to re-export from index.ts by adding exportFromIndex: true to your config file. I am intentionally not exposing it to users due to discussed above.

@mrlubos
Copy link
Member

mrlubos commented Dec 19, 2024

@josstn do you mind creating a new issue and describe what other functions you're using + ideally provide a snippet of your custom code? thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature 🚀 New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants