fix: useKeyMap unmount removes keys (3778)

[changeset-bot[bot]]

⚠️ No Changeset found

Latest commit: 2b8e9e8633dc5ecf93198a2ce08901ee636e0f65

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[linux-foundation-easycla[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: patomation / name: Patrick Kelly (2b8e9e8633dc5ecf93198a2ce08901ee636e0f65)

[isomx]

I don't think this does anything to fix the problem. The original issue was that the keyMap is added as an Object, but then removed via each individual key, which this PR still does.

I can't verify 100% because I can't build this locally at the moment, but iirc from my original tests, editor.removeKeyMap() checks by reference, and thus this will fail to locate the original that you're trying to remove because each key of the Object is not the same as the Object itself.

Did you check that after calling removeKeyMap() the originally registered keys are actually removed from the editor?

Sorry I can't build locally right now to check myself.

[isomx]

I should be back to where I can take a look either late tonight or tomorrow (US-Eastern Time) and I'll post a plan

[patomation]

I don't think this does anything to fix the problem. The original issue was that the keyMap is added as an Object, but then removed via each individual key, which this PR still does.

I can make a change to remove the keys by object instead of individual key. Should we also support individual keys as well? Or should we use objects. I see the function type accepts both.

Sorry I can't build locally right now to check myself.

As for the build issues. @isomx are you seeing this error?

TypeError: Cannot read properties of undefined (reading 'DocExplorer')

[isomx]

Ok, had a chance to pull it up. And no, I'm not seeing that DocExplorer error, I just wasn't where I could access a development environment. But yeah, I've had a litany of build issues with this, it's crazy. It took me the better part of 1/2 a day to finally get it to compile and render in the app. It was extremely frustrating for something that should've just been a drop in.

But to the problem at hand...

The crux of the issue is the fundamental misunderstanding of how add/removeKeyMap works.

It seems that you guys mistook the fact that removeKeyMap() accepts a string that they mean the string representing the commands.

So you loop over each command and provide that to removeKeyMap.

But that's not what they mean by "name", and so that does nothing.

They mean a name property of the keyMap Object. Meaning, you can name the map.

A keyMap can only be removed in 2 ways:

1. Providing an exact reference of the originally registered keyMap - same as removing an event listener on an HTML element by providing the original callback.
2. Setting keyMap.name = "myName" before passing to addKeyMap(), and then calling removeKeyMap("myName") to remove it.

None of the usages of useKeyMap are setup to handle keeping a reference to their registered keyMap.

And worse, some don't even memoize their keys Array passed to useKeyMap, which causes the keyMap to be re-added on each individual render.

I routinely see editor.state.keyMaps grow to > 1000 entries with very little time using the editor.

The easiest fix would be to concatenate the keys provided to useKeyMap and set that as the name property of the keyMap, which you could then use to remove it without having the original reference.

Like this:

function useKeyMap(editor, keys, callback) {
  React.useEffect(() => {
    if (!editor) {
      return;
    }
    // Optionally namespace them in addition 
    // to using the commands. And optionally 
    // use a separator. I'm using `___`, but
    // it doesn't matter, or even none at all.
    const name = `graphiql__${keys.join('___')}`;
    editor.removeKeyMap(name);
    if (callback) {
      const keyMap = { 
        // Set name to be used for later removal.
        // But if the `keys` change, this won't
        // work, so it's not a perfect fix. But
        // it will at least be an improvement since
        // it looks like most usages provide the
        // same keys each time, even if they don't
        // memoize them. 
        name 
      };
      for (const key of keys) {
        keyMap[key] = () => callback();
      }
      editor.addKeyMap(keyMap);
    }
  }, [editor, keys, callback]);
}

[acao]

@isomx you now need to build-bundles as well as yarn build to run e2e and unit tests against graphiql workspace

thanks everyone for working on this! I hope to release a resolution of some kind this week, once I have more time to focus on it