Support binary result format by msand · Pull Request #3316 · brianc/node-postgres

charmander

Choose a reason for hiding this comment

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

Thanks for the bug fix in the serializer.

Tests?

const len = reader.int32()
// a -1 for length means the value of the field is null
fields[i] = len === -1 ? null : this.reader.string(len)
fields[i] = len === -1 ? null : textMode ? reader.string(len) : reader.bytes(len)

Choose a reason for hiding this comment

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

Should this use the field’s mode?

The constructor also still has

if (opts?.mode === 'binary') {
  throw new Error('Binary mode not supported yet')
}

Choose a reason for hiding this comment

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

Agreed that mode is incorrect here.

This may be fixed by #3495