Open MaryGao opened 1 month ago
So the generic parameter here for union type is providing the lower boundary of those type variants, I cannot say I like it because in majority of cases, it is not really helpful.
I kind of like a solution that we update the SdkCredentialType
itself so that, the type of this parameter is just simply SdkCredentialType
, and the SdkCredentialType
itself could represent that we have multiple choices and which are them.
Such as:
export interface SdkCredentialParameter extends SdkModelPropertyTypeBase {
kind: "credential";
type: SdkCredentialType;
onClient: true;
}
export interface SdkCredentialType extends SdkTypeBase {
kind: "credential";
supportedSchemes: HttpAuth[];
}
It is a bigger breaking change, but I think this makes more sense.
Thanks for Dapeng's suggestion! There are some other information(link) of SdkUnionType<SdkCredentialType>
giving us like isGeneratedName, name and crossLanguageDefinitionId etc. Actually JS would not leverage this information because of native support of union, but not sure this would be helpful for other languages.
@tadelesh @iscai-msft I'd like to hear your suggestions here!
Here we have two options: Option 1
export interface SdkCredentialParameter extends SdkModelPropertyTypeBase {
kind: "credential";
type: SdkCredentialType | SdkUnionType<SdkCredentialType>; // union of credentials
onClient: true;
}
Option 2
export interface SdkCredentialType extends SdkTypeBase {
kind: "credential";
supportedSchemes: HttpAuth[];
}
As we offline discussed Option 1 could better present the client type for credential information(credential is either a credential or a union of credentials). And Option 2 would explicitly present credential type but with implicit union meaning here, it is a list of supported schemas.
So personally I would vote for Option 1 for a clear type information.
@tadelesh @iscai-msft I notice we also talked about the endpoint parameter changes not sure there is similar issue with credential parameter. I'd like to hear your insight.
@MaryGao I like option 1 as well. I think we can type the SdkUnionType
as SdkUnionType<T1, ..., Tn>
to represent a union with the item types T1, …, Tn. You don't need an entry for every single value
in SdkUnionType
, just one for each possible type
.
@MaryGao I like option 1 as well. I think we can type the
SdkUnionType
asSdkUnionType<T1, ..., Tn>
to represent a union with the item types T1, …, Tn. You don't need an entry for every singlevalue
inSdkUnionType
, just one for each possibletype
.
So SdkUnionType
does not have a value
, I assume you are referring values
(which I believe is a really bad name, they are not values).
Even if you are doing this, we must have values
.
For instance, even for built-in types, each instance of SdkBuiltInType
with a specific kind
could be totally different types, such as we have a union of SdkUnionType<SdkBuiltInType, SdkBuiltInType>
from the spec:
model Foo
{
prop: string | azureLocation;
}
in the values we have:
kind: "union"
values: [
{
"kind": "string",
"crossLanguageDefinitionId": "TypeSpec.string", // this represent it is string
},
{
"kind": "string",
"crossLanguageDefinitionId": "Azure.Core.azureLocation" // this is azure location
}
]
Or you could think how the type A | B
below looks like:
model Foo
{
prop: A | B;
}
model A {}
model B {}
The generic parameter helps, but we still cannot remove the values
.
@iscai-msft Could you expand your idea of SdkUnionType<T1, ..., Tn> a little? I may be not aware that we have features to spread generic arugements(see issues).
Back to Option 1, if we have possible values with SdkCredentialType or SdkEndpointType we may use |
like SdkUnionType<SdkCredentialType | SdkEndpointType>.
In SdkUnionType
we would list the possible types that .values
could be. This follows your options 1, it just expands it to also allow for multiple types.
model MyModel {
prop: string[] | string;
}
Would result in an SdkUnionType<Array<string>, string>
Request Change
Here is the definition of SdkCredentialParameter and it contains a union of
SdkUnionType
which will not be scoped intocredential
kind. But in fact any detailed type of crednetial parameter should becredential
kind. When traversing the union items we need to castcade to SdkCredentialType.Proposal
So my proposal is we could make SdkUnionType generic. And then in SdkCredentialParameter we could pass SdkCredentialType in union type.
With this change we could better narrow down the type info with credential only. But please note this would be a breaking change for emitters.
Checklist