feat(window)!: brand ListenerId so off() cannot take an event name - #2007
Open
AngelPaella wants to merge 3 commits into
Open
feat(window)!: brand ListenerId so off() cannot take an event name#2007AngelPaella wants to merge 3 commits into
AngelPaella wants to merge 3 commits into
Conversation
🦋 Changeset detectedLatest commit: f6f7847 The changes in this PR will be included in the next version bump. This PR includes changesets to release 18 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
AngelPaella
force-pushed
the
fix/eng4-360-listener-ids
branch
from
August 6, 2026 00:42
37d5ccd to
a949f37
Compare
Contributor
🔥 Smoke Test Results❌ Status: Failed Statistics
Test DetailsThis is a non-blocking smoke test. Full regression tests run separately. |
AngelPaella
marked this pull request as ready for review
August 6, 2026 18:01
Contributor
|
Reviews (1): Last reviewed commit: "fix(window): make off() reject event nam..." | Re-trigger Greptile |
Contributor
|
Reviews (2): Last reviewed commit: "fix(react-ui): make the OAuth flow own i..." | Re-trigger Greptile |
AngelPaella
force-pushed
the
fix/eng4-360-listener-ids
branch
from
August 10, 2026 17:42
e914efe to
d3b4f7e
Compare
AngelPaella
changed the base branch from
main
to
fix/eng4-360-oauth-popup-ownership
August 10, 2026 17:42
Contributor
|
Reviews (3): Last reviewed commit: "fix(window): make off() reject event nam..." | Re-trigger Greptile |
AngelPaella
force-pushed
the
fix/eng4-360-listener-ids
branch
from
August 10, 2026 17:58
d3b4f7e to
e1e7b7e
Compare
AngelPaella
changed the title
AngelPaella
changed the base branch from
fix/eng4-360-oauth-popup-ownership
to
fix/eng4-360-listener-call-sites
August 10, 2026 17:58
Contributor
|
Reviews (4): Last reviewed commit: "feat(window)!: brand ListenerId so off()..." | Re-trigger Greptile |
`on()` returns a listener id and `off()` takes one, but both were `string`, so
`off("ui:height.changed")` type-checked, looked up an event name in a map keyed
by random ids, and silently removed nothing. Four call sites did exactly that.
`ListenerId` is now branded and `off()` only accepts one, making the mistake a
compile error. The brand sits on `Transport.addMessageListener()` /
`removeMessageListener()` rather than only on `EventEmitter`, so ids are branded
where they are minted instead of laundered with a cast in `on()`. That also
closes a hole: `SignersWindowTransport` and `RNWebViewTransport` are publicly
exported, so a consumer calling `removeMessageListener(eventName)` directly got
the same silent no-op.
Every in-repo call site already passes real ids after #2014 and the call-site PR,
so this commit touches no call sites. Consumers holding an id in a `string`-typed
variable have to switch to the exported `ListenerId`.
client-sdk-base (`PaymentMethodManagementIFrameEmitter`,
`EmbeddedCheckoutV3IFrameEmitter`, `IdentityVerificationIFrameEmitter`) and
client-sdk-rn-window (`WebViewParent`) re-expose the narrowed signature through
their public types, so they are majors too rather than taking an automatic patch
that would break consumer builds with no semver signal.
Two AssertTrue lines pin the guarantee at build time: widening `off()` back to
`string`, or dropping the brand from `ListenerId`, fails the dts build.
AngelPaella
force-pushed
the
fix/eng4-360-listener-ids
branch
from
August 10, 2026 18:06
e1e7b7e to
5be82a1
Compare
Contributor
|
Reviews (5): Last reviewed commit: "feat(window)!: brand ListenerId so off()..." | Re-trigger Greptile |
Review feedback from Devin on the brand's public surface. `Transport` is a public interface, so branding its return type pushed an `as ListenerId` cast onto every implementor, including our own three transports. `mintListenerId()` is now the sanctioned constructor and the single remaining cast in the codebase; WindowTransport, SignersWindowTransport and RNWebViewTransport all route through it. client-sdk-base and client-sdk-rn-window re-expose the narrowed `off()` through their emitter types, and the changeset tells consumers to switch to the exported `ListenerId`. Neither package re-exported that type, so a consumer of either had to add a direct dependency on client-sdk-window to name it, which strict resolvers would not let them do transitively. Both now re-export it, so the migration the changeset describes is actually reachable. Not taken: making the brand per-transport. An id minted by one transport still type-checks against another's `removeMessageListener`. Fixing that needs a phantom type parameter threaded through EventEmitter and every emitter alias, for a mistake nobody has made, since transports are 1:1 with their emitter.
Contributor
|
Reviews (6): Last reviewed commit: "refactor(window): add mintListenerId and..." | Re-trigger Greptile |
Contributor
|
Reviews (7): Last reviewed commit: "Merge branch 'fix/eng4-360-listener-call..." | Re-trigger Greptile |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Now the type change only: zero call-site edits. Last in the stack, behind #2014 and #2015.
Correcting my earlier claim on this PR that the migration was "atomic by construction" — that was wrong. It is only atomic if the brand lands first. Land it last and each leak fix ships independently as a patch, which is what #2014 and #2015 now do.
The bug
on()returns a listener id,off()takes one, and both werestring:So
off("ui:height.changed")type-checked, looked up an event name in a map keyed by 13-char random ids, and silently removed nothing. Four call sites did exactly that.ListenerIdis now branded andoff()only accepts one, so the mistake is a compile error.Why this ships last
The leaks are already in prod, so there is no reason to hold the fixes behind a breaking change. This PR is a major for three packages, which is a release-coordination event; #2014 and #2015 are patches that can ship on the next normal release.
By the time this lands every in-repo call site already passes a real id, so this commit touches no call sites. Verified: with #2014 and #2015 in the base,
pnpm build:libsis 16/16 green with no component or hook changes in this diff.Branding at the mint point
The brand sits on
Transport.addMessageListener()/removeMessageListener(), not only onEventEmitter, so ids are branded where they are created instead of laundered withas ListenerIdinsideon(). That also closes a hole:SignersWindowTransportandRNWebViewTransportare publicly exported, so a consumer callingremoveMessageListener(eventName)directly still got the silent no-op this whole line of work is named after.Breaking scope
client-sdk-base(PaymentMethodManagementIFrameEmitter,EmbeddedCheckoutV3IFrameEmitter,IdentityVerificationIFrameEmitter) andclient-sdk-rn-window(WebViewParent) re-expose the narrowedoff()through their public types, so they are majors too. Left implicit they would have taken an automatic patch and broken consumer builds with no semver signal.Migration for consumers: anything holding an id in a
string-typed variable switches to the exportedListenerId.Verification
The guarantee is compile-time, so the check is too. Two lines in
EventEmitter.ts:Mutation-tested, each fails the dts build:
off(id: string)->TS2344: Type 'false' does not satisfy the constraint 'true'type ListenerId = string-> same, on both linespnpm build:libs16/16,pnpm test:vitest11/11 packages,pnpm lintclean.