microsoft / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
163.9k stars 29.17k forks source link

vscode.deprecated.d.ts #88391

Closed mjbvz closed 4 years ago

mjbvz commented 4 years ago

Problem

As the VS Code api has expanded, we have marked a few APIs as deprecated to discourage their use. We have generally committed to support these old apis so that already published extensions will not break, even on newer VS Code versions. However, we do want extension to migrate off of these deprecated apis and we do not want new extensions using the deprecated APIs

By keeping the deprecated apis in our main vscode.d.ts, it may not be clear to extension authors what the best practices are in terms of API usage. An extension author may also now know when a better API comes along to replace one of the apis they were using

Proposal

To address these issues, I propose we investigate gradually moving deprecated APIs from our main vscode.d.ts into a new vscode.deprecated.d.ts file. We would continue to support these deprecated APIs just like we do now.

This would have a few benefits:

JacksonKearl commented 4 years ago

Would it make sense to have potentially many of these vscode.deprecated.d.ts files? As is the very moment you want one deprecated feature, you import vscode.deprecated.d.ts and will have access to all of them in Intellisense, and will receive no further indications of future deprecations in compiler warnings/etc.

Maybe we could either roll them out by version of deprecation (vscode.deprecated.v12.d.ts, vscode.deprecated.v13.d.ts, ...) or by deprecation area (vscode.deprecated.multiRoot.d.ts, idk what else)?

mjbvz commented 4 years ago

Just to toss out another idea:

At the other extreme, we could not publish any vscode.deprecated.d.ts and instead just delete deprecated apis from vscode.d.ts. Extensions could stick on old vscode.d.ts versions if they want to use a deprecated API

(we'd still likely use a vcode.deprecated.d.ts file internally)

rebornix commented 4 years ago

My 2 cents: If the deprecated API has alternatives, we can remove it from vscode.d.ts and provide good migration hints/guidances when extensions are being developed. If there are no alternatives at all, I prefer current situation as there isn't anything an extension author can do other than unpublishing the extension.

jrieken commented 4 years ago

With vscode.proposed.d.ts we have seen cased where not all definitions can be spread across different files, eg when it comes to constructors. We are then often pragmatic and make changes in vscode.d.ts and not in .proposed.d.ts. I am afraid that we will have the same issues with .deprecated.d.ts.

When it comes to breaking we have made a clear statement that we don't break the API. However, there is "binary compatibility" and "source compatibility", e.g "an extension written a long time ago still runs" or "an extension written a long time ago still compiles". That is two different things and we do break source compatibility from time to time, usually when being more expressive with TypeScript, e.g ReadonlyArray or strict null support. I would argue that removing deprecated APIs from vscode.d.ts is breaking source compatibility only.

Still, is it worth the effort? In total there are just 14 deprecations. Binary compatibility won't allow us to remove implementations. To make matters more complicated we still need another, full definition file because we need that as compile-time check for the API implementation.

How about tackling this from the tooling side? Not too long ago we have added SymbolTag.Deprecated and if TypeScript would adopt this we can add squiggles/strikeouts when using deprecated APIs. We can give deprecated symbols a bad rank in IntelliSense. Finally, other TS/JS code would also benefit from this.

mjbvz commented 4 years ago

Good points.

Just deleting deprecated functions would workaround would simplify things since we would not have to worry about splitting the API between files. As @rebornix notes though, that may confuse extension authors (although I'm not sure it would be much worse in practice than moving symbols to a vscode.deprecated.d.ts, which would also cause compile errors)

Here's the use case that inspired this: I would like to update provideCodeActions so that it only returns a CodeAction type instead of a Command | CodeAction. I can document that you should use CodeAction and add runtime warnings if you return a command, but I really want it so that new extensions only are aware that they can return a CodeAction. (this particular API is also something that is not currently marked @deprecated)

We have two TS proposals for deprecation: https://github.com/microsoft/TypeScript/issues/33093 https://github.com/microsoft/TypeScript/issues/33092 We could push for TS to prioritize these if we feel we have a solid use case. At very soonest, they would likely be in the TS 4.0 timeframe (six months out)

mjbvz commented 4 years ago

Telemetry showed that our built-in extensions were using deprecated APIs :)

Looking over other numbers, both the scm.inputBox and Task.constructor apis have under a 100 machines that are using them so far. Other ones have more active machines.

We should discuss what we want to do with this information at the next API sync. I'm still wanting to remove the typings that let code action providers return Commands (while keeping runtime support for converting commands -> code actions)

mjbvz commented 4 years ago

Closing since we now have good deprecated support from TypeScript

Additionally, as part of this issue we added telemetry and warnings for deprecated apis where possible