mariadb-corporation / mariadb-connector-nodejs

MariaDB Connector/Node.js is used to connect applications developed on Node.js to MariaDB and MySQL databases. MariaDB Connector/Node.js is LGPL licensed.
GNU Lesser General Public License v2.1
366 stars 91 forks source link

Make result set's meta property non-enumerable #217

Closed urugator closed 1 year ago

urugator commented 1 year ago

The result set's meta info is rarely needed, therefore I appreciate it's exposed to the user in a way that doesn't complicate the API for majority of use cases. Likewise I think it shouldn't make day-to-day debugging and testing more complicated. As it is now: 1) It clutters console output with a lot of unneeded information. 2) It fails test assertions against plain arrays (using jest)

As a result a user is forced to create copy or redefine the prop every time, which is inconvinient and may have some perf implications.

For these reasons, I propose to make meta property non-enumerable, solving both of the stated problems.

rusher commented 1 year ago

I tend to agree with that. there would be 2 options concerning metadata :

example : with current implementation:

const res = await connection.query('select * from animals');
console.log(res):
// [
//    { id: 1, name: 'sea lions' }, 
//    { id: 2, name: 'bird' }, 
//    meta: [ ... ]
// ]

A better solution would be to still have meta property, but nonenumerable:

const res = await connection.query('select * from animals');
console.log(res):
// res : [
//    { id: 1, name: 'sea lions' }, 
//    { id: 2, name: 'bird' }
// ]
const meta = res.meta;
//    meta: [ ... ]
urugator commented 1 year ago

Didn't know about metaAsArray, seems not to be documented (on github) or part of type defs.

I would vote for metaEnumerable even though it means a new option. Plus a note in docs that false will be default in next major (?).

rusher commented 1 year ago

exactly my point.

Documentation is always a work in progress. thanks for pointing that out. I've just added it in develop branch: https://github.com/mariadb-corporation/mariadb-connector-nodejs/blob/develop/documentation/promise-api.md#metaasarray

MR4online commented 1 year ago

@rusher I think the docs could be wrong, or I missunderstood it.

current docs say:

const [rows, meta] = await connection.query({ metaAsArray: true, sql: 'select * from animals' });
// rows = [ 
//    [ 1, 'sea lions' ], 
//    [ 2, 'bird' ],
// ]
// meta = [...]

shouldn't it be like:

const [rows, meta] = await connection.query({ metaAsArray: true, sql: 'select * from animals' });
// rows = [ 
//    {'id': 1, 'name': 'sea lions' }, 
//    {'id': 2, 'name': 'bird' },
// ]
// meta = [...]

because in my test the "root" result object now is correctly sepperated in an array, but the data is still json (which i love). When reading the docs I thought its all arrays now.

urugator commented 1 year ago

@MR4online Yes, looks like a copy-paste mistake (copied from rowsAsArray)

rusher commented 1 year ago

yes, that is now corrected ( in commit https://github.com/mariadb-corporation/mariadb-connector-nodejs/commit/ba70356ae9485120120b57843f9157ac83145f04 )

rusher commented 1 year ago

precision, metaEnumerable is not handled in batch command, this will be corrected in 3.1.1

rusher commented 1 year ago

closing since 3.1.1 is released