Skip to content
This repository has been archived by the owner on Jun 19, 2024. It is now read-only.

add { signal } option #3

Merged
merged 1 commit into from
May 16, 2024
Merged

add { signal } option #3

merged 1 commit into from
May 16, 2024

Conversation

juliangruber
Copy link
Member

@juliangruber juliangruber commented May 16, 2024

This allows iteration to be canceled cleanly.

Blocks filecoin-station/spark-api#304

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea to add support for AbortSignal 👍🏻

I am unsure if (re)throwing the AbortError is the right way to handle the signal. I am concerned that this could cause unhandled rejections in the code consuming on-contract-event.

As a user of this library, I would expect the library to silently stop listening for new events when the abort signal is triggered. (I.e. treating "abort" as "stop listening").

Another aspect to consider: I think we should eventually implement a timeout for RPC API calls so that the HTTP request does not hang forever if the server is slow to respond. Such improvement is out of the scope of this pull request, though.

Anyhow, I am fine to land this PR as-is and iterate on improving the DX for consumers.

@juliangruber
Copy link
Member Author

I am unsure if (re)throwing the AbortError is the right way to handle the signal. I am concerned that this could cause unhandled rejections in the code consuming on-contract-event.

As a user of this library, I would expect the library to silently stop listening for new events when the abort signal is triggered. (I.e. treating "abort" as "stop listening").

I have thought about this approach, but realized fetch() throws on abort, and to me this is the classic JS platform abortable function. I don't know whether it's necessarily the right approach for iteration too, but I have a feeling it's better because it lets the consumer choose whether to handle or ignore the event.

In spark-api we're handling it like this:

;(async () => {
  for await ...
})().catch(console.error)

Another aspect to consider: I think we should eventually implement a timeout for RPC API calls so that the HTTP request does not hang forever if the server is slow to respond. Such improvement is out of the scope of this pull request, though.

Good idea!

Anyhow, I am fine to land this PR as-is and iterate on improving the DX for consumers.

@juliangruber juliangruber merged commit 14dbeb9 into main May 16, 2024
1 check passed
@juliangruber juliangruber deleted the add/signal branch May 16, 2024 11:31
@juliangruber
Copy link
Member Author

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants