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

fix/support multiple tags per endpoint #82

Closed
SeanCassiere opened this issue Jul 22, 2022 · 11 comments · Fixed by #84
Closed

fix/support multiple tags per endpoint #82

SeanCassiere opened this issue Jul 22, 2022 · 11 comments · Fixed by #84
Assignees

Comments

@SeanCassiere
Copy link
Collaborator

I was trying out the package and noticed that each endpoint is limited to a single tag.
Please see below.

const testRouter = createRouter().query("hello", {
	meta: { openapi: { enabled: true, method: "GET", path: "/hello", tag: "OnlyASingleTag" } },
	input: z.object({ /** input stuff */ }),
	output: z.object({ /** output stuff */ }),
	async resolve({ input }) {
		/** implementation stuff */
	},
});

Maybe this needs to change? Since tags are used to group operations together and a single one could be in two groups. Also, Swagger Docs on the OpenAPI Spec for 3.0.3, states that the tag field should container an Array of string tags. -> string[]
https://swagger.io/specification/#operation-object

@SeanCassiere
Copy link
Collaborator Author

Looking into the codebase, this change seems quite doable.

The only user-facing change I forsee, is to rename the tag property to tags, as now it would take an array of strings instead.

Would look a little something like this.

meta: { openapi: { enabled: true, method: "GET", path: "/hello", tags: ["TagA", "TagB"] } }

I'll submit a PR in a few.

@jlalmes
Copy link
Owner

jlalmes commented Jul 22, 2022

Hi @SeanCassiere, thanks for opening an issue. How would you feel about tag: string | string[]?

@SeanCassiere
Copy link
Collaborator Author

Hi @jlalmes, sounds good to me. Was just about to publish and put a PR 😄.

In one of my commits I updated the examples (express and Nextjs) to use Arrays, shall I revert back to original or use
tag: ["auth"]?

@jlalmes
Copy link
Owner

jlalmes commented Jul 22, 2022

Alternatively with could support: { tag?: string } | { tags?: string[] }? Or should we just go with the OpenAPI spec, as you suggested (bearing in mind this will be a breaking change).

EDIT: I quite like the DX of tag, I rarely ever need to assign more than one tag to an endpoint.

@SeanCassiere
Copy link
Collaborator Author

SeanCassiere commented Jul 22, 2022

To avoid making it a breaking change, { tag?: string } | { tags?: string[] } makes a lot of sense, it'd be more like just adding a feature. Using tag: string | string[] would also work well, since your userbase will be us Typescript nerds and this typing will be easily visible without having to introduce a new property.

In the future, you may want to consider only supporting tags, for the sake of sticking to the OpenAPI spec.

Let me know your preference, I'll make the changes accordingly.

@jlalmes
Copy link
Owner

jlalmes commented Jul 22, 2022

To avoid making it a breaking change, { tag?: string } | { tags?: string[] } makes a lot of sense

Let's do this - leave your changes in the examples if you've already made them. Thanks! 🙌

@SeanCassiere
Copy link
Collaborator Author

SeanCassiere commented Jul 22, 2022

@jlalmes Got a Typescript question... is there a better way of doing this?

type OpenApiMetaProps = {
  enabled: boolean;
  method: OpenApiMethod;
  path: `/${string}`;
  summary?: string;
  description?: string;
  protect?: boolean;
};

interface OpenApiMetaStringTag extends OpenApiMetaProps {
  tag?: string;
}

interface OpenApiMetaArrayTag extends OpenApiMetaProps {
  tags?: string[];
}

export type OpenApiMeta<TMeta = Record<string, any>> = TMeta & {
  openapi?: OpenApiMetaStringTag | OpenApiMetaArrayTag;
};

EDIT: Meaning, is there a way of doing the types where it won't slow you down in the future?

Or would something simpler like this work better?

export type OpenApiMeta<TMeta = Record<string, any>> = TMeta & {
  openapi?: {
    enabled: boolean;
    method: OpenApiMethod;
    path: `/${string}`;
    summary?: string;
    description?: string;
    protect?: boolean;
    tag?: string;
    tags?: string[];
  };
};

@jlalmes
Copy link
Owner

jlalmes commented Jul 22, 2022

I'd probably do something like this:

export type OpenApiMeta<TMeta = Record<string, any>> = TMeta & {
  openapi?: {
    enabled: boolean;
    method: OpenApiMethod;
    path: `/${string}`;
    summary?: string;
    description?: string;
    protect?: boolean;
  } & ({ tag?: string } | { tags?: string[] });
};

@SeanCassiere
Copy link
Collaborator Author

I'd probably do something like this:

export type OpenApiMeta<TMeta = Record<string, any>> = TMeta & {
  openapi?: {
    enabled: boolean;
    method: OpenApiMethod;
    path: `/${string}`;
    summary?: string;
    description?: string;
    protect?: boolean;
  } & ({ tag?: string } | { tags?: string[] });
};

Nice! thanks 👍🏼

@SeanCassiere
Copy link
Collaborator Author

Needed to make a small tweak to the types but now it works, and I've just made the commit.

export type OpenApiMeta<TMeta = Record<string, any>> = TMeta & {
  openapi?: {
    enabled: boolean;
    method: OpenApiMethod;
    path: `/${string}`;
    summary?: string;
    description?: string;
    protect?: boolean;
  } & ({ tag?: string; tags?: never } | { tag?: never; tags?: string[] });
};

For the docs, I've currently just changed it like this. Thoughts on the wording? @jlalmes

Property Type Description Required Default
tag string A single tag used for logical grouping of endpoints in the OpenAPI document. false undefined
tags string[] A list of tags used for logical grouping of endpoints in the OpenAPI document. false undefined

@jlalmes
Copy link
Owner

jlalmes commented Jul 22, 2022

Looks good to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants