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

vscode.deprecated.d.ts #88391

Closed
mjbvz opened this issue Jan 9, 2020 · 7 comments
Closed

vscode.deprecated.d.ts #88391

mjbvz opened this issue Jan 9, 2020 · 7 comments
Assignees
@jrieken @mjbvz
Labels
api under-discussion Issue is under discussion for relevance, priority, approach

Comments

@mjbvz
Copy link
Contributor

mjbvz commented Jan 9, 2020

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:

  • When an author starts a new extension, they would never see the deprecated APIs and patterns

  • Extension authors would be notified when we deprecate an API (due to a compile time error after updating to a new vscode.d.ts version)

  • It would slightly condense our main vscode.d.ts file

  • Extensions that really wish to continue using deprecated APIs could simply import vscode.deprecated.d.ts
@JacksonKearl
Copy link
Contributor

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
Copy link
Contributor Author

mjbvz commented Jan 9, 2020

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
Copy link
Member

rebornix commented Jan 10, 2020 •
edited

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
Copy link
Member

jrieken commented Jan 10, 2020

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
Copy link
Contributor Author

mjbvz commented Jan 10, 2020

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: microsoft/TypeScript#33093 microsoft/TypeScript#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 mjbvz added api under-discussion Issue is under discussion for relevance, priority, approach labels Jan 11, 2020
mjbvz added a commit to mjbvz/vscode that referenced this issue Jan 14, 2020
@mjbvz
Add common service for logging deprecated api usage
7e85242 
For microsoft#88391

Adds a new `ExtHostApiDeprecationService`. This service logs a warning and telemetry when a deprecated API is used.

Updates some of the more simple deprecated apis to use this new service
mjbvz added a commit to mjbvz/vscode that referenced this issue Jan 14, 2020
@mjbvz
Add common service for logging deprecated api usage
0fa8a81 
For microsoft#88391

Adds a new `ExtHostApiDeprecationService`. This service logs a warning and telemetry when a deprecated API is used.

Updates some of the more simple deprecated apis to use this new service
@mjbvz mjbvz mentioned this issue Jan 14, 2020
Merged
mjbvz added a commit to mjbvz/vscode that referenced this issue Jan 14, 2020
@mjbvz
Add common service for logging deprecated api usage
15e168c 
For microsoft#88391

Adds a new `ExtHostApiDeprecationService`. This service logs a warning and telemetry when a deprecated API is used.

Updates some of the more simple deprecated apis to use this new service
mjbvz added a commit that referenced this issue Jan 14, 2020
@mjbvz
Add common service for logging deprecated api usage (#88585)
3883ebd 
* Add common service for logging deprecated api usage

For #88391

Adds a new `ExtHostApiDeprecationService`. This service logs a warning and telemetry when a deprecated API is used.

Updates some of the more simple deprecated apis to use this new service

* Note that extensionId cannot be undefined

* Fix event name
alexr00 added a commit that referenced this issue Jan 22, 2020
@alexr00
Adopt deprecation service for deprecated task constructor
c3a5ac0 
Part of #88391
@mjbvz mjbvz mentioned this issue Feb 13, 2020
Closed
3 tasks
mjbvz added a commit that referenced this issue Feb 13, 2020
@mjbvz
Use non-deprecated version of registerTasksProvider
ea08806 
For #88391
@mjbvz
Copy link
Contributor Author

mjbvz commented Feb 13, 2020

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)

@Ikuyadeu Ikuyadeu mentioned this issue Apr 10, 2020
Closed
@mjbvz
Copy link
Contributor Author

mjbvz commented Oct 14, 2020

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

@mjbvz mjbvz closed this as completed Oct 14, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Dec 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@jrieken jrieken

@mjbvz mjbvz

Labels
api under-discussion Issue is under discussion for relevance, priority, approach
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@rebornix @jrieken @JacksonKearl @mjbvz and others