-
Notifications
You must be signed in to change notification settings - Fork 42
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
base: main
Are you sure you want to change the base?
Conversation
All changed packages have been documented.
Show changes
|
You can try these changes here
|
@@ -479,6 +480,12 @@ export interface SdkPathParameter extends SdkModelPropertyTypeBase { | |||
correspondingMethodParams: SdkModelPropertyType[]; | |||
} | |||
|
|||
export interface SdkCookieParameter extends SdkModelPropertyTypeBase { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- 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 - 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.
- 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.
There was a problem hiding this comment.
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.
resolve: #1792