Conversation
withListeners API for IoHost
withListeners API for IoHostwithListeners API for IoHost
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1708 +/- ##
==========================================
- Coverage 91.30% 91.03% -0.28%
==========================================
Files 79 79
Lines 12164 11819 -345
Branches 1719 1683 -36
==========================================
- Hits 11106 10759 -347
- Misses 1023 1026 +3
+ Partials 35 34 -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:
|
| */ | ||
| 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.
There was a problem hiding this comment.
Update, it exists now and it is separate. If it extended IIoHost the intersection would restate what the host already provides and flatten the caller's own type.
| * 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.
There was a problem hiding this comment.
Exactly, I think a proxy is the right solution here!
| 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.
There was a problem hiding this comment.
But we now have IIoMessageMatcher!
There was a problem hiding this comment.
It went the other way, the interface is deleted. Makers are callable now, so a maker is the type guard directly and there is nothing left for the interface to do. One signature per method on the registry.
mrgrain
left a comment
There was a problem hiding this comment.
Also some changes have been made on main that solve dispose much nicer. We should include them here.
| * 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.
Technically this needs to be IIoHostWithListeners but like I've said, I don't see why we need a combined interface. IoEmitter might still be the better name even if we extend.
There was a problem hiding this comment.
Dropped the combined interface. IoEmitter holds the six registration methods and does not extend IIoHost,
|
|
||
| /** | ||
| * A message matcher | ||
| * | ||
| * Decides whether a message matches a listener, narrowing its payload type `T`. | ||
| */ | ||
| export interface IMessageMatcher<T> { | ||
| is(msg: IoMessage<unknown>): msg is IoMessage<T>; | ||
| } | ||
|
|
||
| /** | ||
| * A request matcher. | ||
| * | ||
| * Carries the response type `U` so an answer can be typed. | ||
| */ | ||
| export interface IRequestMatcher<T, U> extends IMessageMatcher<T> { | ||
| is(msg: IoMessage<unknown>): msg is IoRequest<T, U>; | ||
| } |
There was a problem hiding this comment.
Do these Matchers need to be generic? What functionality are we gaining?
There was a problem hiding this comment.
The generic only existed to carry the payload type and the makers do that themselves nowthat they are callable so deleted both. io-message.ts has no changes in this PR any more.
| on<T>( | ||
| matcher: IMessageMatcher<T>, | ||
| listener: (msg: IoMessage<T>) => MessageListenerResultOrPromise, | ||
| ): () => void; | ||
| on( | ||
| selector: IoMessageCode | MessagePredicate, | ||
| listener: (msg: IoMessage<unknown>) => MessageListenerResultOrPromise, |
There was a problem hiding this comment.
So we allow 3 different types here. Can we limit this? MessagePredicate and IMessageMatcher seem to be the same thing. Even IoMessageCode we can get rid of if we provide a code matcher.
There was a problem hiding this comment.
Down to one.
export type MessageMatcher = (msg: IoMessage<unknown>) => boolean;
byCode replaced the IoMessageCode arm as per suggestion. Payload typing survives without a second arm because a narrowing matcher is just a type guard, and a type guard is assignable to MessageMatcher.
| // The shared listener engine. Registration and message transformation live | ||
| // here; this host does its own I/O (writing, prompting, telemetry, observers) | ||
| // around `registry.apply`. See `on`/`once`/`rewrite`/`respond`. | ||
| private readonly registry = new ListenerRegistry(); |
There was a problem hiding this comment.
What is this registry? If we need it, this tells you that our wrapping pattern doesn't work as designed.
There was a problem hiding this comment.
this comment reframed the whole PR for me. CliIoHost is wrapped like any other host now and a declaration merge keeps every existing call site compiling.
There was a problem hiding this comment.
Please check which types need exporting from here. Also maybe some are duplicated with public types, not sure.
There was a problem hiding this comment.
Went through them, and it is down to two exports, matchAny and ListenerRegistry.
| 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.
But we now have IIoMessageMatcher!
| * Remove every listener registered via `on`/`once`/`rewrite`/`respond`, | ||
| * keeping the host's internal listeners so the host keeps working afterwards. | ||
| */ | ||
| public removeUserListeners(): void { |
There was a problem hiding this comment.
To be fair, I'd rather get rid of the internal concept completely...
…tener-api # Conflicts: # packages/aws-cdk/lib/cli/io-host/cli-io-host.ts
|
Total lines changed 1498 is greater than 1000. Please consider breaking this PR down. |
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 private
ListenerRegistryand adds a publicwithListeners(host)wrapper, so listeners can be attached to anyIIoHost:A listener can observe a message (
on/once), rewrite its text and level (rewrite/rewriteOnce), suppress it, or answer a request (respond/respondOnce). Each registration returns a disposer that works both as a call and underusing. Documented in the toolkit-lib README under#### Attaching listeners to an IoHost.Nine additions users interact with:
withListeners(host), which returns the host you passed in, typedT & IoEmitter.IoEmitteris the six registration methods it adds.MessageMatcher, the one way to select messages, andbyCode<T>(...codes)to build one from one or more message codes.MessageListenerResultandMessageListenerResultOrPromisefor what a listener may return (message,level,action,preventDefault,respond), andDisposeListenerfor what registering gives back.RespondOptionsandRewriteOptions, the options bags forrespond/respondOnceandrewrite/rewriteOnce, replacing positional booleans and levels.Listeners are keyed on a
MessageMatcher, which is any(msg: IoMessage<unknown>) => boolean. A matcher that carries a payload type is just a type guard, sobyCode<StackDetailsPayload>('CDK_TOOLKIT_I2901')typesmsg.datawith nothing named in between. This PR also makes the message makers callable, soIO.CDK_TOOLKIT_I2901is itself a matcher, and because a maker narrows all the way toIoRequest<T, U>,respond's value is checked against the request's response type. That is whyIMessageMatcher,IRequestMatcher,MessagePredicateandIoHostWithListenersare deleted rather than published, and whyListenerRegistrytakes exactly one signature per method, with the typed signatures declared once inIoEmitter.withListenersis a Proxy, which is what keeps the inner host's own methods, getters, setters and full type intact, and wrapping is idempotent so there is never a second registry double-handling a message. Four of the proxy's rules are load-bearing rather than cosmetic, around bindingthisfor hosts with#privatefields, letting an own property shadow the additions, keeping the additions offObject.keys, and capturing the innernotifyat wrap time so a later override cannot recurse. Each has a test inlisteners.test.tsnaming the failure it prevents.CliIoHostno longer owns a registry, it is wrapped like anything else.CliIoHost.instance()hands outattachListeners(new CliIoHost(props)), andexport interface CliIoHost extends IoEmitter {}merges the added methods into the class type, so every call site incdk-toolkit.tsis textually unchanged whilecli-io-host.tsloses 505 lines and gains 107. Three things fall out of that re-layering. ThecorkReplayingflag is gone, because the cork buffer now sits below the listener layer instead of replaying back throughnotify, which also fixes a latent double-count where telemetry saw a corked message twice. Listeners lose their privileged tier, soremoveAllListeners,addInternalandremoveUserListenersare deleted and stack-activity routing registers with plainon. And telemetry ordering is guaranteed rather than incidental, since it registers first and a laterpreventDefaultcan no longer stop it counting a dropped message.One existing seam needed plumbing rather than porting.
CliIoHostalready hadobserveMessages, which reports the emitted message, its effective form, and whether a listener dropped it, all three for one message, and the snapshot recorder intest/_helpers/io-recorder.tsis built on it. It could compute that itself while the listeners ran inline. Now that they run a layer down, it cannot, so the privateattachListenerstakes an optionalListenerVerdictHookand the CLI forwards it into the same fan-out.ObservableIoHost,observeMessagesandIoMessageObservationall survive, the last as an alias forListenerVerdict, andio-recorder.tsis untouched. The hook is private because it only makes sense for a host at the bottom of the stack that wants the whole stream, and it is fixed at wrap time rather than added later, which is why passing it to an already-wrapped host throws.Two things change in observable behavior, both intended:
preventDefaulton a request with no answer now throws. A request's declared default is often approval, so resolving with it would approve on the user's behalf. PairpreventDefaultwithrespond, or userespond, which sets both. No CLI path reaches this, since every@suppressMessagestarget is aninfoortracenotification.oncelistener is claimed before it is awaited, so concurrent emissions (e.g. parallel stacks) can no longer double-fire it.Flagging two changes. Message makers gained
.isrecently in #1679 and this PR drops it in favour of calling the maker, which is what makes the matcher interfaces unnecessary, at the cost of overloadingIO.Xas a factory via.msg()and a predicate when called. AndremoveAllListenersis deleted, which after #1887 had only test call sites.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license