-
Notifications
You must be signed in to change notification settings - Fork 6
Extend EventEmitter #80
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
base: main
Are you sure you want to change the base?
Conversation
tmcw
commented
Oct 8, 2025
- Fixes Extend EventEmitter if we want event-style interfaces #79
Switch to using Undinci pool
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
* fix: Close requests pools on termination * Add changeset
* Don't include tests in dist * Ignore better * Fix biome formatting * Add changeset --------- Co-authored-by: maxmcd <[email protected]> Co-authored-by: Tom MacWright <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
* Don't close the pool This caused failures in prod for us: there are still messages being sent at this point. * Add changeset
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
* Remove configurable spawn function * Expand on changeset * Biome fix * Remove type export
🦋 Changeset detectedLatest commit: df31c24 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
404Wolf
left a comment
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 makes sense given how we are doing things currently!
I will caution that this gives us less control because we can't "await" the emits. I.e. if we emit, and then close the pool, and the things we emit rely on the pool being open.
| }); | ||
|
|
||
| await jsonRequest(worker, "http://localhost"); | ||
| await worker.terminate(); |
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.
Is this desired behavior? Maybe we should throw so we understand why we are terminating already terminated workers. But this is how it was previously (I think), so maybe it's fine.
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.
Terminate has code that looks like it's trying to make it safe to call repeatedly:
deno-http-worker/src/DenoHTTPWorker.ts
Lines 308 to 311 in 66a43ea
| async _terminate(code?: number, signal?: string) { | |
| if (this.#terminated) { | |
| return; | |
| } |
So I just want to, for any function that we want to treat as safe to call like that (idempotent, more or less) to confirm that they really are.