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

[tcgc] add SdkCookieParameter type and support @cookie in TypeSpec http lib #1812

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

Conversation

tadelesh
Copy link
Member

@tadelesh tadelesh commented Nov 6, 2024

resolve: #1792

@azure-sdk
Copy link
Collaborator

azure-sdk commented Nov 6, 2024

All changed packages have been documented.

  • @azure-tools/typespec-client-generator-core
Show changes

@azure-tools/typespec-client-generator-core - feature ✏️

add SdkCookieParameter type and support @cookie in TypeSpec http lib

@azure-sdk
Copy link
Collaborator

You can try these changes here

🛝 Playground 🌐 Website 📚 Next docs

@@ -479,6 +480,12 @@ export interface SdkPathParameter extends SdkModelPropertyTypeBase {
correspondingMethodParams: SdkModelPropertyType[];
}

export interface SdkCookieParameter extends SdkModelPropertyTypeBase {
Copy link
Member

Choose a reason for hiding this comment

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

Quoted @lirenhe's comment below so my understanding is TCGC would not create a cookie parameter type for now?

TCGC could throw warning and don't expose it until we get feedback from service team.

Copy link
Member Author

Choose a reason for hiding this comment

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

i don't think tcgc should swallow any parameter. emitter could choice whether ignore it.

Copy link
Contributor

Choose a reason for hiding this comment

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

this might cause issues in consuming emitters, just because since we're adding another type here, so previous switch statements / if else checks might be affected. I'm ok with this though, we should just make sure people are aware.

Copy link
Member

Choose a reason for hiding this comment

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

We need to figure out what is acceptable in the future but I think a crash because you don't handle a new type is ok as long as this new type doesn't show up on existing specs. If the TypeScript build fails because of it its also a different problem from the runtime failing.
In general I think the following needs to be true when working with ts unions for us to keep adding new features:

  1. adding a new type to a union shouldn't count as breaking anyone as long as:
    a. the type only show up for scenarios that weren't possible before
  2. if an library TS compilation fails uptaking the new version because they don't handle the new type its a non issue, it doesn't affect costumers.
  3. if the library crash because of the new type(As long as 1.a is respected) , this is not ideal but this is also an acceptable temporary state of the library until that new type is either handled or explicitly made an error/warning and dropped.

Copy link
Member Author

Choose a reason for hiding this comment

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

agree. i'll let all language emitter's dev to confirm this change.

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.

Cookie support in tcgc
5 participants