Version 8.5.0 fails with ssl: true

[npalmiusfutr]

I'm trying to use a connection with the following configuration in pg v8.5.0

{
  ...
  ssl: true
}

but I get the following error:

.../pg/lib/connection.js:86
        if ('key' in self.ssl) {
                  ^
TypeError: Cannot use 'in' operator to search for 'key' in true
    at Socket.<anonymous> (.../pg/lib/connection.js:86:19)
    at Object.onceWrapper (events.js:422:26)
    at Socket.emit (events.js:315:20)
    at Socket.EventEmitter.emit (domain.js:486:12)
    at addChunk (_stream_readable.js:309:12)
    at readableAddChunk (_stream_readable.js:284:9)
    at Socket.Readable.push (_stream_readable.js:223:10)
    at TCP.onStreamRead (internal/stream_base_commons.js:188:23)

Node v14.15.0. Using pg v8.4.2 is fine.

[brianc]

uhhh that's not good I'm sorry about that. Lemme see if I can repro & get a patch out.

[brianc]

I'm not able to reproduce - could you expand the code example a bit more?

[npalmiusfutr]

Hmm, strange - will try.

[brianc]

Is it possible you're passing the string 'true'?

[npalmiusfutr]

Yes, I just came to that conclusion too - I was indeed passing the string, which worked in v8.4.2, but doesn't in v8.5.0. I think this is probably working as expected though.

The reason that I ended up passing the string instead of the boolean is rather convoluted, but essentially I was setting it as an environment variable which was then being passed directly through as a string. I can easily fix my code, but this does seem like a breaking change.

[brianc]

yeah i will try to coerce it to a bool lemme see if i can do that

[brianc]

fix here

[npalmiusfutr]

Nice one, thanks for the super quick response and fix. Apologies for the scare!

[brianc]

no, no problem. you're right it's a subtle backwards compat break - strings being coerced to booleans and all that is hard to catch every edge case. Got a regression test for it going forward though.

[brianc]

once the tests pass i'll push a new patch version out

[npalmiusfutr]

Great stuff, thanks again! Yeah, what js dev hasn't been caught out by something like this at some point!?

[charmander]

Might be nice to avoid supporting non-libpq strings in a future major version, since "false" doesn’t have a matching behaviour (right?).

[brianc]

Might be nice to avoid supporting non-libpq strings in a future major version, since "false" doesn’t have a matching behaviour (right?)

Right. I agree supporting true was a bad design choice from the outset.

[npalmiusfutr]

I guess it probably wasn't a conscious design choice as such, just something that happened to work... I wouldn't object to support for the string value being removed (but might be nice to have an 'invalid value' error to make it easier to track down resulting issues).

[brianc]

I wouldn't object to support for the string value being removed

yeah, any time we actually intentionally remove anything we go through a formal and somewhat drawn out deprecation w/ warning period first. A semver minor we'd never intentionally make anything not backwards compatible. It's just...as you saw...kinda difficult to catch everything because javascript is so flexible and sometimes a bit cray cray. 🙃

[npalmiusfutr]

Sounds good @brianc, thanks again for your help. Don't worry, I completely understand how these things easliy creep in! It's difficult to predict all the obscure ways that users like me will end up relying on javacript magic to abuse your package's interface...

[dhruv-singhal-github]

same problem persists for require as have to pass require as non-string