feat(toolkit-lib): withListeners API for IoHost#1708
Conversation
withListeners API for IoHost
withListeners API for IoHostwithListeners API for IoHost
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1708 +/- ##
==========================================
- Coverage 89.59% 89.41% -0.18%
==========================================
Files 77 77
Lines 11758 11552 -206
Branches 1651 1624 -27
==========================================
- Hits 10534 10329 -205
Misses 1195 1195
+ Partials 29 28 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Total lines changed 1101 is greater than 1000. Please consider breaking this PR down. |
There was a problem hiding this comment.
Nope, these are private on purpose. Do not export them.
There was a problem hiding this comment.
Done. MessageInfo, CodeInfo, ActionLessMessage, ActionLessRequest are all un-exported again, and IO and the maker types are off the public barrel too. Nothing maker internal is public now
| */ | ||
| export type MessageSelector<T> = | ||
| | IoMessageMaker<T> | ||
| | IoRequestMaker<T, any> |
There was a problem hiding this comment.
These are not public types, nor should they be. We can accept string here (a single code) or a new MessageMatcher class (via IMessageMatcher). We can make the MessageMakers implement that interface so we can keep passing them in.
There was a problem hiding this comment.
Went with the string option. The public selector is now IoMessageCode (a single code) or a (msg) => boolean predicate, so it no longer references the maker types. I tried the IMessageMatcher route first but dropped it. To keep the typed maker path it wanted, IO would have to be public, and exporting IO pulls the maker internal types back onto the public surface through the same forgotten export cascade with api extractor. With IO private there's also no way for an external caller to obtain an IMessageMatcher. CliIoHost keeps passing makers internally through the private registry unchanged.
There was a problem hiding this comment.
yes there is, the CLI is a privileged caller that can use private exports from toolkit-lib.
In either case I don't understand why it's not possible. Can you share the IMessageMatcher interface you attempted?
There was a problem hiding this comment.
Sorry, my earlier statement was off. I got there by trying to give public users a typed msg.data without a cast, and I talked myself into thinking that needed IO to be public. It doesn't. A public matcher interface is fine on its own, and only exporting IO itself would leak the maker types. Here's the interface, it's on the branch:
export interface IMessageMatcher<T> {
is(msg: IoMessage<unknown>): msg is IoMessage<T>;
}
export interface IRequestMatcher<T, U> extends IMessageMatcher<T> {
is(msg: IoMessage<unknown>): msg is IoRequest<T, U>;
}The makers implement it so internally the CLI keeps passing them and gets typed data and the maker types stay private.
But the typing goal I was chasing doesn't actually resolve. Internally the CLI can use the matcher because it has the makers. A public user has no matcher to pass, since IO and the makers are private. To get one they'd either need us to export IO, or hand-write the matcher:
host.on({ is: (m): m is IoMessage<StackDetailsPayload> => m.code === 'CDK_TOOLKIT_I2901' }, (m) => m.data.stacks);But that m is IoMessage<StackDetailsPayload> doesn't give much over just casting on the code path:
host.on('CDK_TOOLKIT_I2901', (m) => (m.data as StackDetailsPayload).stacks);So the typed matcher only seems to pay off for the CLI. For public users it's a similar cast. Given that, does it earn a place on the public surface, or should the matcher be internal and leave the public API as IoMessageCode | MessagePredicate?
| */ | ||
| export type MessageSelector<T> = | ||
| | IoMessageMaker<T> | ||
| | IoRequestMaker<T, any> |
There was a problem hiding this comment.
yes there is, the CLI is a privileged caller that can use private exports from toolkit-lib.
In either case I don't understand why it's not possible. Can you share the IMessageMatcher interface you attempted?
| * ``` | ||
| */ | ||
| rewrite( | ||
| code: IoMessageCode, |
There was a problem hiding this comment.
added. missed it earlier.
| * const dispose = host.respond('CDK_TOOLKIT_I7010', true); | ||
| * ``` | ||
| */ | ||
| respond(code: IoMessageCode, value: unknown, suppressQuestion?: boolean): () => void; |
There was a problem hiding this comment.
I know is copied from what we currently have, but for a public API we need to think more careful about the design. If we ever want to add an other option this gets messy. We can preemptively put suppressQuestion in a property bag (options).
There was a problem hiding this comment.
Updated. It's a RespondOptions object now, so we can add more options later without changing the signature.
| export function withListeners(host: IIoHost): IoHostWithListeners { | ||
| return new ListeningIoHost(host); | ||
| } |
There was a problem hiding this comment.
Not sure I love this, but I guess it doesn't hurt either. 🤷🏻
| * codes are listed in the message registry: | ||
| * https://docs.aws.amazon.com/cdk/api/toolkit-lib/message-registry/ | ||
| */ | ||
| export interface IoHostWithListeners extends IIoHost { |
There was a problem hiding this comment.
do we need to extend the IIoHost interface? Or is IoEmitter a separate independent interface?
There was a problem hiding this comment.
I held off IoEmitter because I wasn't sure of the benefit over the wrapper for our cases. The one place it would help is reusing one emitter across many hosts. If that is a expected case, then this would be small addition.
| * const toolkit = new Toolkit({ ioHost: host }); | ||
| * ``` | ||
| */ | ||
| export function withListeners(host: IIoHost): IoHostWithListeners { |
There was a problem hiding this comment.
We have this, we can at least make this a generic type so that the inner host keeps its higher fidelity.
There was a problem hiding this comment.
I dug into this and I'm not sure I found the right answer, so I'd value your read.
The generic form that keeps the host's full type is withListeners<T>(host: T): T & IoHostWithListeners. My worry is that the wrapper only implements notify, requestResponse, and the listener methods, so I think that type would promise the host's other members while they'd actually be undefined at runtime.
I tried two ways to make it honest and neither quite felt right:
- A Proxy forwarding unknown members to the inner host. Reads seem fine, and a set trap would probably cover writes, but when I wrapped a host that already has a registry (like
CliIoHost) I ended up with two registries both firing on notify, and I couldn't find a trap that resolved it cleanly. - Adding the listener methods straight onto the host and handing it back. That one is genuinely the host, but it changes the object that was passed in, and since the CLI host is a shared singleton I think that would affect everyone.
So for now I've gone back to returning IoHostWithListeners. It still extends IIoHost, which is all the toolkit consumes, and the caller still holds their own typed reference to the host they passed in, so it seems like they don't really lose it. If there's an approach you had in mind that avoids these I'd really appreciate the guidance. I couldn't reason one that holds up.
| public on<T>( | ||
| selector: IoMessageMaker<T> | IoRequestMaker<T, any> | ((msg: IoMessage<any>) => msg is IoMessage<T>), | ||
| listener: (msg: IoMessage<T>) => MessageListenerResultOrPromise, | ||
| ): () => void; | ||
| public on( | ||
| predicate: (msg: IoMessage<any>) => boolean, | ||
| listener: (msg: IoMessage<unknown>) => MessageListenerResultOrPromise, | ||
| ): () => void; | ||
| public on(selector: MessageSelector<any>, listener: MessageListenerFn): () => void { |
There was a problem hiding this comment.
since this is a private class, it feels strange to have so many different signatures. But I guess thats because we couldn't make IIoMessageMatcher yet.
There was a problem hiding this comment.
yes, and now the selector went from two maker arms plus a predicate down to a matcher or a predicate.
The CLI's
CliIoHosthas a listener mechanism that's private and welded to that one class. Programmatic toolkit-lib users have no way to observe or reshape what flows through their IoHost short of writing a whole custom host.This PR extracts that engine into a shared
ListenerRegistryand adds a publicwithListeners(host)wrapper, so listeners can be attached to anyIIoHost:A listener can observe a message (
on/once), rewrite its text (rewrite/rewriteOnce), suppress it, or answer a prompt (respond/respondOnce), and each registration returns a disposer.CliIoHostis refactored to delegate to the sameListenerRegistry, so there's one implementation of matching/ordering/rewriting/request-answering instead of two.The additions users interact with:
withListeners(host)and its return typeIoHostWithListeners.IMessageMatcherandIRequestMatcherpass one to key a listener by message and get a typed msg.data.MessagePredicatefor matching on anything other than a single code.RespondOptionsforrespond/respondOnce.MessageListenerResult,MessageListenerResultOrPromiseListeners are keyed on a message
codestring (IoMessageCode, e.g.'CDK_TOOLKIT_I2901'), aMessagePredicateor aIMessageMatcher. With a code or predicatemsg.dataisunknownand you cast it to the interface documented in the message registry. With a matcher,msg.datais typed.Because
CliIoHostnow shares the extracted engine, one thing changes on the CLI path, intended: aoncelistener is removed before it is awaited, so concurrent emissions (e.g. parallel stacks) can no longer double-fire it.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license