Make IAction properties readonly
#164846
Make IAction properties readonly
#164846
Conversation
The `IAction` interface is primarily used to passing around information about actions. Many properties on this interface are currently writable however, which is not ideal as it means callers are free to change these properties in sometimes unexpected ways Worse, the few places in our codebase that do need to write these properties assume that they actually have an `Action` instead of an `IAction`. You can see this happening with [`MenuItemAction`](https://github.com/microsoft/vscode/blob/9c802415e11bf11230720bc4a93397c338121da8/src/vs/platform/actions/common/actions.ts#L401), which declares all of its properties as `readonly` but can then can see these properties be mutated if it is passed as an `IAction` In order to prevent problems like the above, and also to hopefully increase the number of places where we use simple `IActions` instead of the more heavyweight `Action` class, I've updated `IAction` to be the readonly representation of an action. This required updating a few types like `ActionViewItems` to let callers know which type of actions are being used
| @@ -81,11 +80,6 @@ export class TerminalViewPane extends ViewPane { | |||
| ) { | |||
| super(options, keybindingService, _contextMenuService, _configurationService, _contextKeyService, viewDescriptorService, _instantiationService, openerService, themeService, telemetryService); | |||
| this._register(this._terminalService.onDidRegisterProcessSupport(() => { | |||
| if (this._actions) { | |||
There was a problem hiding this comment.
@Tyriar As far as I could tell, this property is unused
There was a problem hiding this comment.
Looks like we removed the usage and forgot to clean up 👍
| @@ -405,12 +399,13 @@ class SingleTerminalTabActionViewItem extends MenuEntryActionViewItem { | |||
| this._register(this._terminalService.onDidChangeInstanceColor(e => this.updateLabel(e.instance))); | |||
| this._register(this._terminalService.onDidChangeInstanceTitle(e => { | |||
| if (e === this._terminalGroupService.activeInstance) { | |||
| this._action.tooltip = getSingleTabTooltip(e, this._terminalService.configHelper.config.tabs.separator); | |||
| // TODO: Fix types | |||
| (this._action as unknown as Action).tooltip = getSingleTabTooltip(e, this._terminalService.configHelper.config.tabs.separator); | |||
There was a problem hiding this comment.
@Tyriar This is the error I mentioned in the PR description. MenuItemAction says its properties are all readonly, but we then try writing them here
There was a problem hiding this comment.
It may be readonly on MenuItemAction but we're dealing with an IAction here (surprised TS never gave us an error for MenuItemAction implements IAction). Changing the tooltip is important here as the terminal title is dynamic, can you come up with a solution for this too so this doesn't just create another debt item that probably won't get looked at for a long time.
| @@ -405,12 +399,13 @@ class SingleTerminalTabActionViewItem extends MenuEntryActionViewItem { | |||
| this._register(this._terminalService.onDidChangeInstanceColor(e => this.updateLabel(e.instance))); | |||
| this._register(this._terminalService.onDidChangeInstanceTitle(e => { | |||
| if (e === this._terminalGroupService.activeInstance) { | |||
| this._action.tooltip = getSingleTabTooltip(e, this._terminalService.configHelper.config.tabs.separator); | |||
| // TODO: Fix types | |||
| (this._action as unknown as Action).tooltip = getSingleTabTooltip(e, this._terminalService.configHelper.config.tabs.separator); | |||
There was a problem hiding this comment.
It may be readonly on MenuItemAction but we're dealing with an IAction here (surprised TS never gave us an error for MenuItemAction implements IAction). Changing the tooltip is important here as the terminal title is dynamic, can you come up with a solution for this too so this doesn't just create another debt item that probably won't get looked at for a long time.
|
Pushed a commit that handles Action safely in |
TylerLeonhardt
added
the
deferred
|
Closing as out of date. May revisit at some point |
The
IActioninterface is primarily used to passing around information about actions. However most properties onIActionare currently writable. This is bad as it makes code more difficult to reason about since callers are free to change these properties in sometimes unexpected ways.Worse, some current implementers of
IActionassume that the properties should are readonly. You can see this happening withMenuItemAction, which declares all of its properties asreadonlybut can then can see these properties be mutated if it is passed as anIActionIn order to prevent problems like the above, and also to hopefully increase the number of places where we can safely use simple a
IActioninstead of the more heavyweightActionclass (see #164842 for an example of why this can be beneficial), I've updatedIActionto be the readonly representation of an action. This also required updating a few types likeActionViewItemsto let callers know which type of actions are being used