[graphiql] useKeyMap() hook does not unregister previous keys

[isomx]

Is there an existing issue for this?

• [X] I have searched the existing issues

Current Behavior

In graphiql-react package, the useKeyMap hook accepts an editor, keys and a callback. When keys change, it calls editor.removeKeyMap(key) for each key in keys.

It then creates an Object representing the new keyMap, and calls editor.addKeyMap(keyMap).

But this results in CodeMirror.state.keyMaps to grow unbounded, because the keys are registered using an Object, but removed using a string which won't match the previous keyMap.

Further, useQueryEditor() does not memoize the keys Array for some of the commands, causing the keys to be re-registered on every render, further populating CodeMirror.state.keyMaps with duplicate entries.

Expected Behavior

Proper removal of previous keyMap before adding a new one, so that CodeMirror.state.keyMaps isn't an Array with 1000s of entries that all represent the same hooks. And also not adding/removing keys in keysMap on every render by properly memoizing keys before calling useKeyMap() in useQueryEditor().

Steps To Reproduce

Any usage of , including

Environment

• GraphiQL Version: 3.7.1
• OS: Windows 10
• Browser: Chrome v128.0.6613.138
• Bundler: Webpack 4
• react Version: 17.0.2
• graphql Version: 16.9.0

Anything else?

No response

[patomation]

I think I'm looking at the correct function.. It looks like it tries to remove the key before it creates one..

But it is not removing them with the un-mount function. can we add a return to the useEffect with something like this?

for (const key of keys) {
   editor.removeKeyMap(key);
}

Would that solve this issue?

[patomation]

How about something like this?

[patomation]

PR #3788

[isomx]

See post on PR, but I don't think this fixes it. Sorry I can't build locally right now to test it myself. So if you verified they actually get removed now, then disregard, I must not be remembering the flow right off the time of my head.