feat: add IPNS publish strategy completion events - #1103
Conversation
tabcat
left a comment
tabcat
left a comment
There was a problem hiding this comment.
@nikhilkumar1612 apologies for the wait. main change i think should be included is replacing #publishStrategySuccess with a function outside of the class called withStrategyProgressEvents that is used to call any router we need to. it wraps the call with start, success, and error events for ipns:publish:strategy namespace. see comments for more detail. otherwise looks pretty good thanks 🙏
| })) | ||
| } | ||
|
|
||
| #strategyName (router: IPNSRouting): string { |
There was a problem hiding this comment.
this is a pure function and doesnt need to exist on the class. can be moved to the bottom on this file as its own function.
| /** | ||
| * The routing key the IPNS record was published under. | ||
| */ | ||
| routingKey: Uint8Array |
There was a problem hiding this comment.
may be better off to include the keyName and publicKey fields here instead of routingKey.
| batch.put(ipnsMetadataKey(routingKey), IPNSPublishMetadata.encode(options.metadata)) | ||
| } | ||
| await batch.commit(options) | ||
| options.onProgress?.(new CustomProgressEvent('ipns:routing:datastore:put')) |
There was a problem hiding this comment.
since we are adding the start/success/failure events for each router call we can remove the changes to this file.
|
|
||
| export type PublishProgressEvents = | ||
| ProgressEvent<'ipns:publish:start'> | | ||
| ProgressEvent<'ipns:publish:strategy:success', IPNSPublishStrategyResult> | |
There was a problem hiding this comment.
lets add ipns:publish:strategy:{start|success|failure} events.
| } | ||
| }) | ||
|
|
||
| this.#publishStrategySuccess('LocalStoreRouting()', routingKey, record, options) |
There was a problem hiding this comment.
can be removed after we call localStore strategy with the withProgressEvents caller.
| } | ||
| } | ||
|
|
||
| #publishStrategySuccess (strategy: string, routingKey: Uint8Array, record: IPNSEntry, options: PublishOptions): void { |
There was a problem hiding this comment.
lets move this out of the class, name it withStrategyProgressEvents and have it call onProgress with start|success|error events.
shape can be something like:
withStrategyProgressEvents (detail: IPNSPublishStrategyDetail, options: ProgressOptions, fn: () => unknown): Promise<void>| } | ||
| }) | ||
|
|
||
| this.#publishStrategySuccess(this.#strategyName(r), routingKey, record, options) |
There was a problem hiding this comment.
can be removed after adding withProgressEvents caller.
There was a problem hiding this comment.
map these to a withProgressEvents call.
There was a problem hiding this comment.
call this with withProgressEvents.
| publicKey: PublicKey | ||
| } | ||
|
|
||
| export interface IPNSPublishStrategyResult { |
There was a problem hiding this comment.
rename to IPNSPublishStrategyDetail
Thanks for the review, I have made the necessary changes. @tabcat |
2 participants
Title
feat: add IPNS publish strategy completion events
Description
Fixes #959.
Adds a publish progress event that fires after each IPNS publishing strategy has completed. This lets callers distinguish between local persistence, Helia routing publication, and any custom routing strategies finishing.
This also addresses the behavior requested in #945 by moving the
ipns:routing:datastore:putprogress event so it is emitted after the datastore batch commit completes. Callers can now wait for the local IPNS record to be persisted without needing to wait for slower network publishing such as DHT publication.Changes include:
ipns:publish:strategy:successput(...)resolvesipns:routing:datastore:putafter local datastore commitNotes & open questions
The new strategy completion event uses a single event type,
ipns:publish:strategy:success, with the completed strategy identified inevt.detail.strategy.Change checklist