Skip to content

Conversation

@apietroni51
Copy link
Contributor

@apietroni51 apietroni51 commented Jun 17, 2025

As suggested by @tarzacodes we have wrapped the original StreamConsumer and StreamPublisher, that expose the manuallyClose parameter for internal reasons (i.e. callbacks), and created a new interface for each that just expose a parameterless close()

@apietroni51 apietroni51 requested review from gpad, l4mby and tarzacodes June 17, 2025 15:29
@apietroni51 apietroni51 linked an issue Jun 17, 2025 that may be closed by this pull request
@albertobarrila
Copy link
Contributor

The .close(false) is called only in the registerForClosePublisher of the client and only for the Publisher otherwise is passed true (that is the default of the param)

I honestly do not like the solution of having a Private that shadow the entire class only to hide a single parameter, maybe is "better" to have a special method to force that param to be called only in that specific case...

something like

// standard method to close the connection
public async close(): Promise<void> {
    if (!this.closed) {
        await this.flush()
        await this.pool.releaseConnection(this.connection)
    }
    this._closed = true
}

// special case where to force that param
// maybe find a better name I'm very bad at it
public async forceClose(): Promise<void> {
    if (!this.closed) {
        await this.flush()
        await this.pool.releaseConnection(this.connection, false)
    }
    this._closed = true
}

@gpad wdyt? @tarzacodes maybe you have already tried this route and found something that I've lost....

@apietroni51 apietroni51 force-pushed the 262-as-a-developer-i-want-to-simplify-the-clientclose-api branch from bb48695 to c21f135 Compare June 25, 2025 09:54
@apietroni51 apietroni51 requested review from albertobarrila and removed request for tarzacodes June 25, 2025 09:54
@apietroni51 apietroni51 force-pushed the 262-as-a-developer-i-want-to-simplify-the-clientclose-api branch from c21f135 to 15ae8d3 Compare June 26, 2025 08:19
albertobarrila

This comment was marked as duplicate.

Copy link
Contributor

@albertobarrila albertobarrila left a comment

Choose a reason for hiding this comment

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

this first solution is a first step towards a simplification (there's more wax to remove) but is ok for now.
I've also swapped ts-node for tsx to support also node 24

@albertobarrila albertobarrila merged commit 70750be into main Jul 1, 2025
4 of 8 checks passed
@albertobarrila albertobarrila deleted the 262-as-a-developer-i-want-to-simplify-the-clientclose-api branch July 1, 2025 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

As a developer, I want to simplify the Client.close() api

3 participants