Tool unregistration design #130
Description
This is a follow-up to #101 (comment), where we agreed you shouldn't be able to unregister a tool that you didn't register, just like you can't unregister an event listener that you didn't register. This means unregisterTool() can't take a simple DOMString, since it's trivial to unregister tools that another script registered.
Let me try and capture a few different design options we have.
1. unregisterTool(ModelContextTool)
This is the simplest approach. It also most closely matches the symmetry between addEventListener() and removeEventListener(), where you need to present the very thing you registered, in order to unregister it. That "thing" is the ModelContextTool dictionary in this case.
This is pretty appealing.
2. registerTool(interface Tool) / unregisterTool(interface Tool)
This is just like the above—you need to present what you registered in order to unregister it—except instead of dealing in dictionaries, we deal in a new Tool interface. You'd now register tools like so, and unregister similarly:
navigator.modelContext.registerTool(new Tool({ … }))This is almost the same as today, but with the new Tool() in there. The benefit is that Tool can inherit from EventTarget, and we can fire the toolactivated and toolcanceled events directly on the tool. I first raised this as a benefit in #76 (comment), since then we could have a policy of always firing these events directly at the tool: the Tool for imperative tools, and the <form> for declarative tools.
I no longer see this as a benefit, and I describe this in #126 (comment). The gist is I think there's no value in firing events directly on the Tool, because anything the tool author would do there they can just do in the actual tool callback. It's not like a click event handler, where the only chance you get to react to the click in script is with the event handler. With imperative tools, the tool callback is the default reaction to tool invocation, so the event handler isn't useful to the tool author.
Rather, these events are probably more relevant to authors of different scripts that want to track all global tool activations/cancelations without having direct access to the tool itself. This is akin to the event delegation pattern on the web today, where some "global" script wants to track all clicks; instead of dumping a million click listeners on a million different <button>s, and mutation tracking each button insertion and removal to add more listeners, the script adds one listener to <body> and consults event.target if needed.
I think with tool execution, the "global" script that might care about all tool activations/cancelation is a concurrency manager that won't have direct access to each tool, or won't want to add event handlers to each form element that might contribute a declarative tool, so firing all of these events at the ModelContext interface itself is probably a good idea, IMO. This is a concern that @bwalderman mentioned in #126 (comment), but with all the discussion here I'm curious if I've convinced him :)
All of this is to say that having a new Tool interface could be nice, but we shouldn't do it if the only motivation is firing events at the tool.
3. registerTool() returns an unregistration thing
This is a blend of a few ideas. It could be what @bwalderman mentioned in #126 (comment), where registerTool() returns a Tool that has an unregister() method. I think it's kinda funny that a method called registerTool() returns the very thing it purports to take as input—a Tool!
const tool = navigator.modelContext.registerTool({ … });
tool.unregister(); // New `Tool` interface has an `unregister()` method.Or this could be what @MiguelsPizza mentions in #101 (comment): where registerTool() returns an unregister() function callback you can invoke to unregister the tool (see also).
const unregister = navigator.modelContext.registerTool({ name: 'my_tool', ... });
unregister();The only downside of this approach is that it's unlike anything else on the platform. Unregistration on the web platform happens either through AbortControllers, or explicit unregistration APIs like removeEventListener(). It wouldn't be the end of the world if we introduced something new, it's just out of step with other APIs, and for no benefit over an explicit unregistration API.
All things considered, I think (1) is the simplest above.