feat: implements unlisten functionality

[stalniy]

Relates to #133

BREAKING CHANGE: listen function returns an object instead of an empty array. All properties are preserved.

[porsager]

I think it'd be great if we could fix subsequent listen calls not receiving the result, in this PR too. Are you up for that? It effects the way unlisten is implemented, but it's odd if only the first listener is able to unlisten.

[stalniy]

Also Fixes #154

[Minigugus]

Great work! And don't forget to update the types 😉

https://github.com/porsager/postgres/blob/master/types/index.d.ts#L340

(I can help with types if you want by the way)

[stalniy]

updated types

[stalniy]

@porsager do you have any feedback after updates?

[porsager]

I'll take a look tonight.

Would you review the types @Minigugus ?

[porsager]

This will result in any listener receiving an unlisten function that only works for the initial listener.

Here's one of the test cases modified to test this.

ot('multiple listeners and unlisten one', async() => {
  const listener = postgres(options)
      , xs = []

  await listener.listen('test', x => xs.push('1', x))
  const s2 = await listener.listen('test', x => xs.push('2', x))
  await listener.notify('test', 'a')
  await delay(50)
  await s2.unlisten()
  await listener.notify('test', 'b')
  await delay(50)
  listener.end();

  return ['1a2a1b', xs.join('')]
})

[stalniy]

Ah, right... unlisten is different for every fn

[stalniy]

fixed

[stalniy]

I changed return type of sql.listen because it seems strange to copy an array with Object.assign but if you would like the return result to be an array, I will do this.

Update: that array is always empty, so very unlikely that anybody relied on it to be an array

[Minigugus]

that array is always empty, so very unlikely that anybody relied on it to be an array

Ok, it's not a bad idea since it hide implementation details and simplify the public API :)

It's also a breaking change, maybe you should also update the changelog 😉

Would you review the types @Minigugus ?

Types looks good :)

[stalniy]

Ok, it's not a bad idea since it hide implementation details and simplify the public API :)

where can I find it?

[Minigugus]

where can I find it?

Ok I thought there was a changelog file, my bad x)

Then there is nothing to, only @porsager is responsible for what is in the changelog on the release page.

[porsager]

I think it's fine that the return type is simply { unsubscribe }.

With regards to the changelog, add it as part of the PR description and then I'll add it to releases.

[stalniy]

I think it's fine that the return type is simply { unsubscribe }.

With regards to the changelog, add it as part of the PR description and then I'll add it to releases.

shall I change the return type?

[porsager]

I don't think there's any use case for having the result included, so if you agree let's do it :)

[stalniy]

@porsager looks like we can't leave unlisten only. there is a test that check listen reconnect and requires connection pid:

t('listen reconnects', async() => {
  const listener = postgres(options)
      , xs = []

  const { state: { pid } } = await listener.listen('test', x => xs.push(x))
  await sql.notify('test', 'a')
  await sql`select pg_terminate_backend(${ pid }::int)`
  await delay(50)
  await sql.notify('test', 'b')
  await delay(50)
  listener.end()

  return ['ab', xs.join('')]
})

[stalniy]

I actually like that it returns pid because I planned to implement similar set of tests on my side :)

[porsager]

Well i guess we're ready then 😊

…

On 5 Mar 2021, at 17:54, Sergii Stotskyi ***@***.***> wrote:  I actually like that it returns a pid because I planned to implement similar set of tests on my side :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[porsager]

That was a little too soon though. The pid will change if the underlying socket reconnects. Let's tackle that in another PR

[stalniy]

Hmm... I think a good way to handle this is to turn pid into a function/getter. So, we do not update this value for every subscription

[porsager]

Yeah, sounds good.

[stalniy]

@porsager how to test unequality or comparison? I'd like to add a test that checks unequality of initial pid and a pid which I get after terminating connection:

t('listen result has state getter', async() => {
  const listener = postgres(options)
      , xs = []

  const result = await listener.listen('test', x => xs.push(x))
  const initialPid = result.state.pid
  await sql.notify('test', 'a')
  await sql`select pg_terminate_backend(${ initialPid }::int)`
  await delay(50)
  listener.end()

  return [result.state.pid, initialPid] // <--- looks like there is no way to check unequality
})

Let's discuss this in #160