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

[TypeScript] Drop support for TS 3.1 #1805

Closed
wants to merge 3 commits into from

Conversation

deyaaeldeen
Copy link
Member

@deyaaeldeen deyaaeldeen commented Sep 15, 2020

It is my understanding that we are not currently supporting TS 3.1 well and no customer needs it. Furthermore, the guidelines is a bit vague in that it says we should support versions > 3.1 (not including 3.1 itself) but also gives tips about how to support TS 3.1. Furthermore, based on a recent discussion in our teams channel, we think it is reasonable to set the minimum version to support to 3.5.

Rationale by @xirzec:

After shipping several Track 2 libraries, we have determined that TS 3.5 is a much more reasonable minimum bar to maintain, because of downlevel-dts support and convenient language features such as Omit. As we have shipped several libraries that require 3.5 or higher without receiving any negative feedback from consumers, we feel confident in updating this requirement to reflect the reality of the ecosystem. We will continue to listen to our customers and revise this guideline in the future as necessary.

Copy link
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

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

The spirit of the change is good, but a PR description would be appreciated.

docs/typescript/design.md Show resolved Hide resolved
@bterlson
Copy link
Member

+1.

The rationale for this is that we have not in practice supported TS3.1 for most libraries for any track 2 release and we have not gotten bugs. At the same time, supporting TS3.1 would be a laborious process.

Copy link
Member

@bterlson bterlson left a comment

Choose a reason for hiding this comment

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

Thanks much for making this PR!!

docs/typescript/design.md Show resolved Hide resolved
@ramya-rao-a
Copy link
Contributor

This PR seems to have come from nowhere...
@deyaaeldeen Can we have a PR description added regarding why we are doing this or which prior conversation lead us here?

@willmtemple
Copy link

Is TS 3.5 an arbitrary choice or do we have some data/observation that this version is important to support?

@deyaaeldeen deyaaeldeen force-pushed the drop-ts-3.1 branch 2 times, most recently from 7e49d95 to 6b7562a Compare September 16, 2020 14:22
@deyaaeldeen
Copy link
Member Author

@ramya-rao-a done.

@willmtemple one thing to note is that downlevel-dts supports down-leveling to TS 3.4 and up, so I think it is much more expensive to support versions earlier than 3.4.

@bterlson
Copy link
Member

I was tempted to say 3.4 because that's what downlevel-dts is attempting to support, but 3.5 added Omit which I find quite handy.

Copy link
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

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

I think I'm good with the changes to the guidelines, but perhaps the description rationale can be cleaned up to something like

After shipping several Track 2 libraries, we have determined that TS 3.5 is a much more reasonable minimum bar to maintain, because of downlevel-dts support and convenient language features such as Omit. As we have shipped several libraries that require 3.5 or higher without receiving any negative feedback from consumers, we feel confident in updating this requirement to reflect the reality of the ecosystem. We will continue to listen to our customers and revise this guideline in the future as necessary.

@southpolesteve
Copy link
Contributor

Is there any telemetry that may help inform this decision? For Cosmos, we have smoke tests down to 3.1 that gate PRs. So it is possible this is breaking for us but I can't say for sure one way or the other.

@deyaaeldeen
Copy link
Member Author

For Cosmos, we have smoke tests down to 3.1 that gate PRs. So it is possible this is breaking for us but I can't say for sure one way or the other.

@southpolesteve I did not know that cosmos has rigorous support for 3.1, thanks for pointing this out! Well, now we are in a state where cosmos has support for 3.1 and the rest not so much, I am not sure about storage though (cc @ljian3377 @XiaoningLiu). One suggestion is to keep the cosmos support for 3.1 and create a plan to deprecate it at some point in the future after having a better understanding of the customer needs through telemetry or other means. On the other hand, we can move other packages to have official support for TS 3.5 and implement a CI check to have the support rigorous similar to what cosmos does. We already have a plan outlined here Azure/azure-sdk-for-js#11319, so for this proposal, cosmos will be taken out of the list of packages to update.

@southpolesteve
Copy link
Contributor

@deyaaeldeen Makes sense. To be clear, I welcome this change. I've added it to our list for v4

@xirzec
Copy link
Member

xirzec commented Sep 28, 2020

I'm curious if anyone has any creative ideas for how we can detect the version of TS folks are using without surveying them. AFAIK there's no good way for us to know if the code that calls us was compiled by TS or not, much less what version of TS they may be using.

@XiaoningLiu
Copy link
Member

For storage SDK, I don't have accurate data. But we can check some internal customers for their status.
Another question is should we treat it as a breaking change for SDK to deprecate 3.1 and upgrade SDK major version?

@deyaaeldeen
Copy link
Member Author

deyaaeldeen commented Sep 29, 2020

@XiaoningLiu thanks for the update! Regarding your question, I believe it depends on the situation of the package. I think if it claimed in the changelog to provide support for 3.1 before, then this is a breaking change (e.g. servicebus has this claim).

@xirzec
Copy link
Member

xirzec commented Sep 29, 2020

Changing supported TS version as a breaking change is interesting. I can see it meriting a minor version bump, but it's a little weird to me to make it a major version bump given that a JS-only customer would be totally immune to the "break".

@weshaggard
Copy link
Member

@deyaaeldeen @xirzec what are the next steps for this PR?

@xirzec
Copy link
Member

xirzec commented Sep 20, 2021

@ramya-rao-a maybe this is a good MQ item?

@ramya-rao-a
Copy link
Contributor

@xirzec Yes. Just like we came up with the support policy for Node.js versions, we should have one for TypeScript and we can do that as part of MQ this time

@deyaaeldeen
Copy link
Member Author

Closing in favor of Azure/azure-sdk-for-js#19843

@deyaaeldeen deyaaeldeen deleted the drop-ts-3.1 branch January 14, 2022 01:14
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.

8 participants