fixing todo by reusing already constructed instances for matching #9874
fixing todo by reusing already constructed instances for matching #9874teslae1 wants to merge 3 commits intoVSCodeVim:masterfrom
Conversation
…ow only constructing new ones on action match - which we need since state gets mutated upon action match construction
|
Hey @J-Fields first of all thanks a lot for your contributions to this extension - like many other developers I rely heavily upon this code everyday. In this pull request I did my best to make a small change that resolves your TODO comment. I was wondering how I would go about getting this reviewed? I ask this because I'm also awaiting a review on #9872 |
|
I am not the package maintainer, but if I may propose an alternative solution: The "TODO" itself mentions making the functions Turn the decorator interface IActionDescription {
keys: readonly string[] | readonly string[][];
modes: readonly Mode[];
actionType: readonly ActionType;
}Declare the argument of Lastly, the issue of inheritance has to be solved, since some actions override the couldActionApply?: (
vimState: VimState,
keysPressed: string[],
base: (vimState: VimState, keysPressed: string[]) => boolean
);An action can then decide to implement this method (with enforced typing!) and it may or may not call the base implementation. This essentially mimics inheritance but through composition instead. Although this requires a substantially larger refactoring than the one in this PR, I think it reduces the memory footprint of checking actions down to zero. |
@MGessinger Thank you for your proposal - I appreciate the time you took.
|
What this PR does / why we need it:
Now only constructs the action on match. The logic for this is now moved to a small wrapper called "ActionMatcher".
Which issue(s) this PR fixes
This pr fixes the TODO: