-
Notifications
You must be signed in to change notification settings - Fork 852
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
A package that makes it easier to work with subscribable objects #3301
A package that makes it easier to work with subscribable objects #3301
Conversation
🦋 Changeset detectedLatest commit: cef0d13 The changes in this PR will be included in the next version bump. 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 |
9c45d54
to
74846f3
Compare
74846f3
to
7ea0591
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! I love that we define our own DataPublisher
type abstraction instead of working directly with event emitters. And I love that the transformation to an async iterable is the very last step, essentially making it an "API decorator".
// It materializes listener signatures based on the events of the emitter | ||
{ | ||
const eventEmitter = null as unknown as TypedEventEmitter<{ foo: CustomEvent<'bar'> }>; | ||
const publisher = getDataPublisherFromEventEmitter(eventEmitter); | ||
publisher.on('foo', data => { | ||
data satisfies 'bar'; | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add a typetest for non-custom events? I'm guessing, based on the type definition below, that null
will be provided as the first argument.
Something like this:
const eventEmitter = null as unknown as TypedEventEmitter<{ foo: SomeEvent }>;
const publisher = getDataPublisherFromEventEmitter(eventEmitter);
publisher.on('foo', (...args) => {
args satisfies readonly [null]; // Is this right?
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oof. This change was done poorly and I didn't typecheck it with the rest of the stack.
7ea0591
to
cef0d13
Compare
it('flushes the queue before vending errors', async () => { | ||
expect.assertions(4); | ||
publish('data', 'consumed message'); | ||
publish('data', 'queued message 1'); | ||
publish('error', new Error('o no')); | ||
await expect(iteratorA.next()).resolves.toStrictEqual({ | ||
done: false, | ||
value: 'queued message 1', | ||
}); | ||
await expect(iteratorB.next()).resolves.toStrictEqual({ | ||
done: false, | ||
value: 'queued message 1', | ||
}); | ||
await expect(iteratorA.next()).rejects.toThrow(new Error('o no')); | ||
await expect(iteratorB.next()).rejects.toThrow(new Error('o no')); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One key difference between this implementation and the last is that the last one would nuke the iterators immediately upon receiving an error. This might have caused code that was behind processing notifications to miss notifications that had already arrived on the client.
This implementation continues to vend any messages in the queue before vending the error. Errors are now a queue member like anything else.
options, | ||
); | ||
return { | ||
async *[Symbol.asyncIterator]() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implementation still uses generators instead of the manual approach cancellation (via return()
) in #3172. This means that the only way to interrupt this generator is to call the abort signal, which aborts the whole subscription – you can't just call .return()
on the iterator. This is probably sufficient for most use cases today, but we should consider rolling in some of the ideas and tests from #3172 anyway.
// It materializes listener signatures based on the events of the emitter | ||
{ | ||
const eventEmitter = null as unknown as TypedEventEmitter<{ foo: CustomEvent<'bar'> }>; | ||
const publisher = getDataPublisherFromEventEmitter(eventEmitter); | ||
publisher.on('foo', data => { | ||
data satisfies 'bar'; | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
Merge activity
|
Summary
At the root of the subscriptions infra is
WebSocket
which conforms to theEventEmitter
interface. In this new package are utilties for making them easier to work with.getDataPublisherFromEventEmitter
lets you convert an emitter into something with anon()
method that vends unsubscribe functions. Unsubscribe functions are much easier to work with because you can pass them around as values to unsubscribe from an event without having to hold on to a reference to the original listener function.createAsyncIterableFromDataPublisher
lets you create anAsyncIterable
from aDataPublisher
. This iterable handles cancellation, queueing when there is backpressure, and error propagation.Read the
README
and the tests for more detail.