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

Update to TypeScript 3.9 #9012

Merged
merged 6 commits into from
Jun 1, 2020
Merged

Update to TypeScript 3.9 #9012

merged 6 commits into from
Jun 1, 2020

Conversation

xirzec
Copy link
Member

@xirzec xirzec commented May 19, 2020

This PR updates TypeScript to the latest version (3.9) to take advantage of compiler speed improvements.

I also fixed a few minor issues where the compiler was being more strict, such as object spread causing the same property to be written twice to an object.

@xirzec xirzec added Client This issue points to a problem in the data-plane of the library. Azure.Core labels May 19, 2020
@xirzec xirzec requested review from bterlson and southpolesteve May 19, 2020 19:15
@xirzec xirzec self-assigned this May 19, 2020
@xirzec xirzec changed the title Update to TS 3.9 Update to TypeScript 3.9 May 19, 2020
@@ -472,6 +472,8 @@ export function padStart(
targetLength: number,
padString: string = " "
): string {
// TS doesn't know this code needs to run downlevel sometimes.
Copy link
Member

Choose a reason for hiding this comment

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

I find this change unexpected (TS3.8 should complain as much) but it should complain and so having the expect error is good.

Copy link
Member

@mikeharder mikeharder left a comment

Choose a reason for hiding this comment

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

Looks fine to me, but I'm not seeing any change in our CI build times.

sdk/keyvault/keyvault-keys/src/index.ts Show resolved Hide resolved
@@ -134,7 +134,7 @@ export class Containers {
};
}

const response = await this.clientContext.create<ContainerRequest>({
const response = await this.clientContext.create<ContainerRequest, ContainerDefinition>({
Copy link
Contributor

Choose a reason for hiding this comment

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

What changed that made it required to specify ContainerDefinition?

Copy link
Member Author

Choose a reason for hiding this comment

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

requests can specify partitionKey with a string (you can see how it gets expanded above before the request is made.)

The server, however, responds with the object form, which we expect. The create wrapper defaults to having the input shape match the output shape... which isn't quite correct as we know we've normalized partitionKey already.

The mismatch in types of partitionKey was blowing up when we construct the ContainerResponse below.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, so TypeScript was complaining as it should have been already?

Copy link
Contributor

Choose a reason for hiding this comment

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

surprised this wasn’t complaining before, but the change and explanation is right

Copy link
Contributor

@chradek chradek left a comment

Choose a reason for hiding this comment

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

Looks good!

@@ -134,7 +134,7 @@ export class Containers {
};
}

const response = await this.clientContext.create<ContainerRequest>({
const response = await this.clientContext.create<ContainerRequest, ContainerDefinition>({
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, so TypeScript was complaining as it should have been already?

@ramya-rao-a
Copy link
Contributor

@southpolesteve Can you take a look as one of the cosmosdb source files is being edited?

@southpolesteve
Copy link
Contributor

Cosmos changes are correct

@ramya-rao-a
Copy link
Contributor

Thanks @southpolesteve

@xirzec Let's wait for someone from the Storage team to sign off as well and then we can merge

@jeremymeng jeremymeng added the Storage Storage Service (Queues, Blobs, Files) label May 20, 2020
@xirzec
Copy link
Member Author

xirzec commented May 26, 2020

@XiaoningLiu @ljian3377 @jiacfan do you have any feedback on this PR? 😄

@xirzec xirzec merged commit ca526b6 into Azure:master Jun 1, 2020
@xirzec xirzec deleted the ts3.9 branch June 1, 2020 20:25
@ramya-rao-a ramya-rao-a mentioned this pull request Jun 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants