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.

Sign up for GitHub

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

Jump to bottom

Upgrade compiler v0.48 #2029

Merged
merged 28 commits into from
Sep 26, 2023
Merged

Upgrade compiler v0.48 #2029

Changes from 1 commit
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
b2bf37f
upgrade the tsp compiler version
MaryGao Sep 19, 2023
d6cb382
Update the breakings
MaryGao Sep 19, 2023
f7b17e8
Merge remote-tracking branch 'origin/main' into upgrade-compiler-v0.48
MaryGao Sep 19, 2023
eafebb6
Fix to pass build errors
MaryGao Sep 19, 2023
77b8a5f
Update the !
MaryGao Sep 19, 2023
d94c5fd
Fix the ut issue in rlc
MaryGao Sep 19, 2023
f7a65f3
Update the test cases
MaryGao Sep 19, 2023
93d6d06
Update the test cases
MaryGao Sep 19, 2023
c0b6f13
update the lib to dev
MaryGao Sep 21, 2023
afdb56f
Update the test cases
MaryGao Sep 21, 2023
b258ae1
Update the spec
MaryGao Sep 21, 2023
4db33a8
Update the lint
MaryGao Sep 21, 2023
748745b
Disable the polling operation setting due to wrong def
MaryGao Sep 21, 2023
d700808
Update .vscode/launch.json
MaryGao Sep 21, 2023
3d4fe46
Update .vscode/launch.json
MaryGao Sep 21, 2023
9beaf7a
Update the error spec
MaryGao Sep 21, 2023
bece042
temp
MaryGao Sep 21, 2023
cd9ee55
temp
MaryGao Sep 21, 2023
2b4b536
update the test cases
MaryGao Sep 22, 2023
0bd3065
Update the test cases
MaryGao Sep 22, 2023
a519249
update changes in openai
MaryGao Sep 22, 2023
35d1d4d
update the statement
MaryGao Sep 22, 2023
f8f749f
Update the modular lro
MaryGao Sep 22, 2023
d2d7d3f
Update the openapi.json
MaryGao Sep 22, 2023
6d83594
Update .vscode/launch.json
MaryGao Sep 22, 2023
a803437
Update packages/typespec-ts/src/modular/buildCodeModel.ts
MaryGao Sep 25, 2023
11fbfdd
update the name
MaryGao Sep 25, 2023
53f4ccc
Update the dependency to dev
MaryGao Sep 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion packages/typespec-ts/test/modularUnit/type.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,10 @@ describe("model type", () => {
assertEqualContent(
modelFile!.getFullText()!,
`
/** Type of ColorType */
export type ColorType = "red" | "blue";
export interface Test {
color: "red" | "blue";
color: ColorType;
Copy link
Member Author

@MaryGao MaryGao Sep 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@iscai-msft @tadelesh Another failed test case is for union of string.

TypeSpec

model Test {
    color: "red" | "blue";
}

Old generation after calling getSdkUnion

/** Type of ColorType */
export type ColorType = "red" | "blue";
export interface Test {
  color: ColorType;
}

Latest generation with getSdkUnion

export interface Test {
        color: "red";
}

After debugging I found the getSdkUnion would only return the first enum member to us. Here is the relevant tcgc code.

image

Copy link
Contributor

Choose a reason for hiding this comment

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

talked with the dpg crew about this, for sdk union types like this scenario, where the definition is `, we've decided to just emit it as a string. This is because there isn't information about the name of the union, and dotnet and java can't handle this. If they want an enum, they should define it as an enum in the tsp file, instead of us creating an enum from it

Copy link
Member Author

@MaryGao MaryGao Sep 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

That's interesting! I think emitting this as a string is not proper for TypeScript because we have native support for string union. And for TypeSpec type "red" | "blue", we may prefer to directly map to TypeScript type "red" | "blue" like below. But currently we only have the first enum in underlying __raw, it makes us impossible to get all values.

On the other hand for other languages they emit as a string but they may have pontential requirement to touch underlying values, e.g using them in descriptions or knownable values etc.

export interface Test {
  color: "red" | "blue";
}

Copy link
Member

Choose a reason for hiding this comment

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

@joheredi Have you been involved in those discussions and are you agree with this ?

Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC we agreed to have it as a string in tcgc since 3/4 languages are just going to emit it as a string, which conflicts with what JS wants here. For JS though, I should be able to fix the __raw to return the original union type and there should be enough information there. Down the line, we might decide that we can toggle this behavior with a flag, but since I think it's very JS specific, I think the best way right now is to have JS get the info they need themselves from the raw type

Copy link
Contributor

@iscai-msft iscai-msft Sep 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Version 0.35.0-dev.5 is currently releasing and has this change

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for the quick fix! Not sure we could have a stable patch version for this?

}`
);
});
Expand Down
Loading