Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
urugator opened this issue Oct 6, 2022 · 8 comments
Closed

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

urugator opened this issue Oct 6, 2022 · 8 comments

Comments

@urugator
Copy link
Contributor

urugator commented Oct 6, 2022

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
Copy link
Collaborator

rusher commented Oct 10, 2022

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

  • existing metaAsArray option for mysql2 compatibility : like const [rows, meta] = conn.query('...');
  • a new option metaEnumerable if needed for compatibility.

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
Copy link
Contributor Author

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
Copy link
Collaborator

rusher commented Oct 11, 2022

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
Copy link

@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
Copy link
Contributor Author

urugator commented Feb 6, 2023

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

@rusher
Copy link
Collaborator

rusher commented Feb 8, 2023

yes, that is now corrected ( in commit ba70356 )

@rusher
Copy link
Collaborator

rusher commented Feb 28, 2023

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

@rusher
Copy link
Collaborator

rusher commented May 2, 2023

closing since 3.1.1 is released

@rusher rusher closed this as completed May 2, 2023
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

No branches or pull requests

3 participants