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

[http] Add Parameter Decorator @cookie to Specify Cookie Parameters #4761

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

su8ru
Copy link

@su8ru su8ru commented Oct 16, 2024

close #4739

This PR adds @cookie decorator to @typespec/http, to decorate parameters as cookie parameters.

@su8ru
Copy link
Author

su8ru commented Oct 16, 2024

I think that @cookie in the return type should be an implicit body (like @path and @query), but I haven't been able to find where this is implemented.
Added a check for isCookieParam() to isMetadata(), but properties with @cookie in the return type are ignored from the content of the response.

@timotheeguerin
Copy link
Member

aren't cookies allowed in response(server asking client to set the following cookies) we might need to design and decide exactly what we want in those cases though

@su8ru
Copy link
Author

su8ru commented Oct 16, 2024

aren't cookies allowed in response

I noticed that cookies can also be allowed or required in responses (e.g. in communication between servers).
This could cause problems if cookies cannot be set, so it might be better to be able to specify cookies in responses.

Copy link
Member

@timotheeguerin timotheeguerin left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution this is awesome! I think there is a little design around the response cookies we'll want to sort out before we merge this but this looks great overall!

packages/http/src/decorators.ts Outdated Show resolved Hide resolved
packages/http/src/decorators.ts Show resolved Hide resolved
context.program.stateMap(HttpStateKeys.cookie).set(entity, options);
};

export function getCookieParamOptions(program: Program, entity: Type): QueryParameterOptions {
Copy link
Member

Choose a reason for hiding this comment

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

would be awesome if you could add some js doc comment to those new functions, we are in the process of documenting all our api

Copy link
Member

@timotheeguerin timotheeguerin Oct 16, 2024

Choose a reason for hiding this comment

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

not asking you to do more work but just so you know this won't add support for it to openapi3 emitter if thats what you were looking for. Would need a few more changes in the packages/openapi3 pkg probably

Copy link
Author

Choose a reason for hiding this comment

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

I tried it in local playground, and it works for me.
It seems there is no specific support for parameters in openapi3, so I think there is no need to add support.
I'll try to add some tests for openapi3 👀

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.

Feature Request: Add parameter decorator @cookie to specify that is in: cookie
2 participants