Add unwrapArgs util

[alexweininger]

Primary motivation for this change is to prevent refactoring commands in the client extensions to handle the added nodes?: T[] argument.

Currently command parameters follow the (context, node?, ...args?) pattern.

We implemented registerCommandWithTreeNodeUnwrapping so that the arguments passed to the command callback would be (context, node?, nodes?, ...args?). Adding the nodes argument in front of the original ...args? is problematic because any command that had arguments must now handle the nodes? argument. Some commands are used by partner extensions via vscode.commands.executeCommand. So on top of having to refactor a bunch of commands, we'd be breaking partner extensions.

To fix this, I'm adding the selected tree items to the context. Since nearly all commands don't currently care about the selected tree items, we can just hide it away in the context. Plus, if VS Code adds any more callback arguments in the future, we can easily add them to the context. I added the tree item to the context too for good measure. In the future we don't have to require commands to follow the (context, node?, ...args?) pattern, instead it could just be (context, ...args?).

All those changes are included in the first commit.

I also did a tiny refactor to registerCommandWithTreeNodeUnwrapping to expose the logic as a standalone util called unwrapArgs. This is needed so we can apply unwrapping where we are using a custom version of registerCommand. For example, we use registerSiteCommand in the App Service extension.

Now we can do registerSiteCommand('appService.Deploy', unwrapArgs(deploy)); to apply unwrapping while still using the existing registration utility.

---

Another solution I thought of but didn't go with in the end: pass in the selected tree items after ...args?

I think this is a fine solution, and it won't require any refactoring or result in breaks for partners. However, it's kinda ugly to always append things to the end of the parameter list. And will get even uglier if VS Code adds additional arguments to the command callbacks.

[bwateratmsft]

We shouldn't be using the registerCommandWithTreeNodeUnwrapping method on all commands in the extensions, only the ones that actually have to do with tree nodes. Also, VSCode is already sending node?: T, nodes?: T[], ...args? (with us prepending context)--so any command that's not handling nodes as the third argument was always incorrectly implemented.

[nturinski]

Do you know if you only have one node selected, does it get passed in as a targetNode or as selectedNodes as a single-entry array?

[bwateratmsft]

Do you know if you only have one node selected, does it get passed in as a targetNode or as selectedNodes as a single-entry array?

Both, I believe.

[alexweininger]

We shouldn't be using the registerCommandWithTreeNodeUnwrapping method on all commands in the extensions, only the ones that actually have to do with tree nodes.

👍

Also, VSCode is already sending node?: T, nodes?: T[], ...args? (with us prepending context)--so any command that's not handling nodes as the third argument was always incorrectly implemented.

Correct, but I think it's better for us to add it to the context instead of refactoring a ton of commands. Most commands don't need to use the selected tree nodes anyway, so it feels like a waste. Especially since we have some commands that other extensions rely on.

Also registerCommandWithTreeNodeUnwrapping now passes in the empty array regardless of how the command is called. But currently VS Code won't pass anything in if the command is called via commands.executeCommand.

[bwateratmsft]

How does putting it into the context make us not have to change commands? Now all the commands that expected the standard argument order (context, node, nodes) have to change.

[alexweininger]

How does putting it into the context make us not have to change commands? Now all the commands that expected the standard argument order (context, node, nodes) have to change.

The commands are getting passed context, node still. I moved the nodes parameter to context.

[bwateratmsft]

It seems like the goal is to solve context, node, ...args, except that's already a wrong pattern because VSCode will be passing in context, node, nodes, ...args for all tree commands. Any commands we currently have with context, node, ...args are implemented wrong.

[alexweininger]

I updated the PR so that it just adds the unwrapArgs util, and removed the other changes related to the selected nodes.