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

Finalize onDidExecuteCommand-API #78091

Closed
jrieken opened this issue Jul 29, 2019 · 4 comments
Closed

Finalize onDidExecuteCommand-API #78091

jrieken opened this issue Jul 29, 2019 · 4 comments
Assignees
Labels
api-finalization under-discussion Issue is under discussion for relevance, priority, approach
Milestone

Comments

@jrieken
Copy link
Member

jrieken commented Jul 29, 2019

Remaining work is listed here: #1431 (comment). Also, finalise the API

@jrieken jrieken added feature-request Request for new features or functionality api-finalization labels Jul 29, 2019
@jrieken jrieken added this to the August 2019 milestone Jul 29, 2019
@jrieken jrieken self-assigned this Jul 29, 2019
@jrieken
Copy link
Member Author

jrieken commented Aug 6, 2019

During the API sync concerns about this API have been raised. The following issues have come up:

  • commands are at the heart of almost everything and there is potential to wreck the extension host when doing something every time a command is run
  • command arguments aren't API, esp. internal objects (like the explorer model) will leak into the extension host setting expectations that we cannot satisfy

/cc @kieferrm

An alternative to having the onDidExecuteCommand-api would be to allow/accept PRs that extend the core of VS Code. Feature requests that would need this API

  • marco recording (run commands in sequence) @hedgerh
  • keybindings tutor (teach keybinding when a command was run)

@jrieken jrieken added the under-discussion Issue is under discussion for relevance, priority, approach label Aug 6, 2019
@jrieken jrieken removed the feature-request Request for new features or functionality label Aug 15, 2019
@jrieken
Copy link
Member Author

jrieken commented Aug 15, 2019

I have pushed 21de711 that removes the proposed API - as explained in my previous comment. This wasn't an easy decision but there were too many doubts around the usefulness of this API, the performance impact, and accidental leakage of arguments.

However, the internal API is there and we are open to accept features like the "keybindings teacher" or "macro recording" as a core contribution to VS Code.

@hedgerh
Copy link
Contributor

hedgerh commented Aug 15, 2019

No worries. Thank you for all your help. I may take a crack at it sometime if I have some time. Cheers.

@moshfeu
Copy link
Contributor

moshfeu commented Sep 26, 2019

Maybe instead of invoking it for each events, a plugin could register to specific events so the callback will be fired only in specific events (PubSub?)

Also, if that API removed, probably it should be mention in the product updates some time.

@vscodebot vscodebot bot locked and limited conversation to collaborators Sep 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-finalization under-discussion Issue is under discussion for relevance, priority, approach
Projects
None yet
Development

No branches or pull requests

3 participants