Implement typed event emitter#869
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes introduce a strongly-typed event system to the mobile SDK alpha package. Event names and payloads are now mapped via a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Listener
participant SDK
Note over Client,SDK: Registering an event listener
Client->>SDK: on('progress', callback)
SDK-->>Client: Unsubscribe function
Note over SDK: Emitting an event
SDK->>Listener: callback(payload) (typed via SDKEventMap)
Note over Client,SDK: Emitting an event externally
Client->>SDK: emit('error', errorPayload)
SDK->>Listener: callback(errorPayload)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/mobile-sdk-alpha/src/client.ts (1)
76-94: Timeout emission can fire after success or cancellation; also risk of unhandled rejections
- The timeout task is never cancelled, so it may emit
'error'even if the proof completes or is cancelled.- If
clock.sleeprejects on abort in some adapter, thevoided promise may trigger an unhandled rejection.Guard emission and catch rejections; also wire cancelation:
async function generateProof( @@ - const timeoutMs = opts.timeoutMs ?? cfg.timeouts?.proofMs ?? defaultConfig.timeouts.proofMs; - void _adapters.clock.sleep(timeoutMs!, opts.signal).then(() => emit('error', new Error('timeout'))); + const timeoutMs = opts.timeoutMs ?? cfg.timeouts?.proofMs ?? defaultConfig.timeouts.proofMs; + let cancelled = false; + void _adapters.clock.sleep(timeoutMs, opts.signal) + .then(() => { + if (!cancelled) emit('error', new Error('timeout')); + }) + .catch(() => { + // aborted: ignore + }); return { id: 'stub', status: 'pending', result: async () => ({ ok: false, reason: 'SELF_ERR_PROOF_STUB' }), - cancel: () => {}, + cancel: () => { + cancelled = true; + }, };Optionally, consider using a domain error (e.g.,
sdkError('SELF_ERR_TIMEOUT', ...)) for consistency.
🧹 Nitpick comments (4)
packages/mobile-sdk-alpha/src/types/public.ts (2)
91-96: SDKEventMap is a solid foundation; consider tightening payload typesCurrent mapping is clear. Two suggestions:
- Consider narrowing
state: stringto a string literal union of valid states to prevent accidental misuse.- If you have an SDK-specific error type (e.g., from
./errors), prefer that over the broadErrorto carry codes/categories consistently.
129-131: Reassess exposingemiton the public client APIExposing
emitpublicly allows external code to spoof internal events (including'error'). If events are intended as output-only signals, consider:
- Keeping
emitinternal (not part ofSelfClient) and exposing onlyon/off/onceto consumers, or- Documenting that
emitis for testing only, or prefixing it with an “internal” marker in docs.
If you keep it public, addingonceandoffwould round out the emitter API ergonomics.packages/mobile-sdk-alpha/src/client.ts (2)
49-55: Listener registry works; minor type-safety nitThe implementation is fine. If you want to avoid
(cb as any), you can model listeners as a keyed record to preserve per-event payload typing:type Listener<K extends SDKEvent> = (payload: SDKEventMap[K]) => void; type Listeners = { [K in SDKEvent]?: Set<Listener<K>> }; const listeners: Listeners = {}; function on<E extends SDKEvent>(event: E, cb: Listener<E>): Unsubscribe { const set = (listeners[event] ??= new Set()) as Set<Listener<E>>; set.add(cb); return () => set.delete(cb); }Not mandatory, but it removes casts and improves maintainability.
101-103: Public emit exposure in returned objectMirrors the interface change. See earlier comment on considering keeping
emitinternal to avoid consumer-triggered synthetic errors.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/mobile-sdk-alpha/src/browser.ts(1 hunks)packages/mobile-sdk-alpha/src/client.ts(4 hunks)packages/mobile-sdk-alpha/src/index.ts(1 hunks)packages/mobile-sdk-alpha/src/types/public.ts(2 hunks)
🔇 Additional comments (2)
packages/mobile-sdk-alpha/src/browser.ts (1)
20-22: Type-only re-export of SDKEventMap is correct and tree-shakeableExporting via
export type { ... }ensures no runtime impact and keeps browser bundles lean. Looks good.packages/mobile-sdk-alpha/src/index.ts (1)
18-20: Consistent public export of SDKEventMapGood to see the event map exposed at the package entrypoint as a type-only export. This keeps external consumers aligned with the new typed event system without bundling overhead.
transphorm
merged commit 2fb33a0
into
codex/simplify-and-fix-alias-import-system
* Simplify alias config * Restore export sorting overrides * save migration pattern * alias last minute updates * fix tests * improved import sorting * Implement typed event emitter (#869) * Implement typed event emitter * cr suggestion
1 participant
Summary
Testing
yarn workspace @selfxyz/mobile-sdk-alpha niceyarn workspace @selfxyz/common nice(fails: Require statement not part of import statement)yarn workspace @selfxyz/app nice(fails: Workspace '@selfxyz/app' not found)yarn lintyarn buildyarn workspace @selfxyz/contracts build(fails: Hardhat config error)yarn typesyarn workspace @selfxyz/common testyarn workspace @selfxyz/circuits test(fails: Unsupported signature algorithm: undefined)yarn workspace @selfxyz/mobile-app testyarn workspace @selfxyz/mobile-sdk-alpha testhttps://chatgpt.com/codex/tasks/task_b_6898c5c2ebe0832db1332f91ff4f4e14
Summary by CodeRabbit
New Features
Refactor