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

Making vscode.executeCommand('vscode.previewHtml', ...) a compile error #62806

Closed
mjbvz opened this issue Nov 8, 2018 · 3 comments
Closed

Making vscode.executeCommand('vscode.previewHtml', ...) a compile error #62806

mjbvz opened this issue Nov 8, 2018 · 3 comments
Assignees
Labels
api *out-of-scope Posted issue is not in scope of VS Code
Milestone

Comments

@mjbvz
Copy link
Contributor

mjbvz commented Nov 8, 2018

Part of #62630

Discourage people from writing the most simple form of the vscode.previewHtml command by making:

vscode.executeCommand('vscode.previewHtml')

A compile error. Obviously this compile time check won't block sneaky code like:

const command = 'vscode.previewHtml'
vscode.executeCommand(command)

or:

vscode.executeCommand('vscode' + '.' + 'previewHtml')

We've already added a runtime warning for development extensions that will catch the above examples and will upgrade this into a user facing warning after a few months

@mjbvz mjbvz added the api label Nov 8, 2018
@mjbvz mjbvz added this to the November 2018 milestone Nov 8, 2018
@mjbvz mjbvz self-assigned this Nov 8, 2018
@mjbvz mjbvz closed this as completed in 96ddd4f Nov 8, 2018
@mjbvz mjbvz added the verification-needed Verification of issue is requested label Nov 10, 2018
mjbvz added a commit that referenced this issue Nov 10, 2018
Use an overload instead of a conditional type to generate a compile error when using `vscode.previewHtml` command

Conditional types were breaking on our build machine
@isidorn isidorn added the verified Verification succeeded label Dec 4, 2018
@isidorn
Copy link
Contributor

isidorn commented Dec 4, 2018

I do not get an error if I use
vscode.commands.executeCommand('vscode.previewHtml');

Am i supposed to use some special version of ts? Or should I be good out of the box?

@isidorn isidorn added verification-steps-needed Steps to verify are needed for verification and removed verified Verification succeeded labels Dec 4, 2018
@mjbvz mjbvz added the *out-of-scope Posted issue is not in scope of VS Code label Dec 4, 2018
@mjbvz
Copy link
Contributor Author

mjbvz commented Dec 4, 2018

Yeah this does not work well using an overload since most cases aren't caught. We have to revert the proper fix, 96ddd4f, because the conditional types broke our build (and would have broken anyone using TS 2.8 or older to target newer VS Code versions).

I'm just going to revert the overload too and mark this as not worth the effort

mjbvz added a commit that referenced this issue Dec 4, 2018
@isidorn isidorn removed verification-needed Verification of issue is requested verification-steps-needed Steps to verify are needed for verification labels Dec 5, 2018
@isidorn
Copy link
Contributor

isidorn commented Dec 5, 2018

@mjbvz ok. Removing verification-needed than

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api *out-of-scope Posted issue is not in scope of VS Code
Projects
None yet
Development

No branches or pull requests

2 participants