refactor: Use maps and sets instead of plain objects.

[gonfunko]

The basics

• I branched from develop
• My pull request is against develop
• My code follows the style guide
• I ran npm run format and npm run lint

The details

Resolves

Part of #5857

Proposed Changes

This PR uses JS Map and Set objects in places where plain objects were being used due to the absence of those data structures in older versions of Javascript. Only variables that were private have been converted.

Reason for Changes

General codehealth and improved typing accuracy.

Test Coverage

Verified tests pass and manually tried the playground.

[maribethb]

some minor suggestions, overall lgtm

[maribethb]

do you really have to deepMerge again after you just created a new one? I think the old one was just to return a copy, but you're already creating a copy so I think this is redundant.

[gonfunko]

Here we do, because there's a test verifying that modifying the returned KeyboardShortcut object doesn't affect the one actually stored in the registry, which it would if we don't used deepMerge to deeply clone it. I don't think it's relevant for getKeyMap() though, so removed it there.

[maribethb]

but isn't legacyRegistry already a clone? because on line 205 you're creating a new object. so i don't see how modifying the return value could affect the value of the underlying this.shortcuts map. but i might be misunderstanding the problem

[gonfunko]

It is, but the KeyboardShortcut objects stored in it aren't without the call to deepMerge, so they could still be mutated by the caller and have that affect the contents of the registry.

[maribethb]

I think this whole thing can be replaced by

if (variableList && arrayUtils.removeElem(variableList, variable)) {
  eventUtils.fire(...);
}

possibly? it's comparing based on the entire VariableModel instead of checking the ids so it's a little different though

(i didn't know arrayUtils existed but we use it right above this so why not)

[gonfunko]

I'm a little leery to do this since it is a semantic change as you noted - going to leave as-is for now, but we might want to revisit this later.

[rachel-fenichel]

I discussed performance of Map and Set with primitive keys with @NeilFraser and the rest of the team.

Some considerations:

• Using an Object instead of a Map or Set is faster (with primitive keys) in Chrome
• The TS style guide says to use Map and Set to avoid unexpected behaviour with Object
• Map and Set improve our typing and the ability of the compiler to check our code
• All of the maps and sets changed by this PR are internal
• Performance matters most on hot paths such as dragging
• From discussion, 3 people prefer to use maps and sets; 2 people prefer to stick with objects

My decision:

• Use Map and Set by default
• Use plain objects on hot paths
• Check whether the compiler transforms it all to Objects under the hood anyway when they're internal-only and use primitive keys

[maribethb]

but isn't legacyRegistry already a clone? because on line 205 you're creating a new object. so i don't see how modifying the return value could affect the value of the underlying this.shortcuts map. but i might be misunderstanding the problem