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

please add an option to get fields information #13

Open
ZaubererMD opened this issue Jan 28, 2019 · 5 comments
Open

please add an option to get fields information #13

ZaubererMD opened this issue Jan 28, 2019 · 5 comments

Comments

@ZaubererMD
Copy link

I want to access the fields property after I have made a query. In the normal async mysql package this would be the third argument passed into the callback of the query-function.

The current implementation always only returns the result-rows. To enable it to return both, the result-rows and the fields, the then-mysql package (also authored by you) would have to be changed as well. In connection.js only a small change would have to be made to the lines 45-47, so when the Promise is resolved it also returns the fields. However, in order not to break existing code, either a second version of the query-method would have to be added which is nearly a clone of the existing one, or you could add a parameter that defaults to the current behaviour, but can be configured so the promise returns e.g. an object like { result, fields }.

With that change in then-mysql, it would be easy to adapt sync-mysql to work with it.

I could write that myself and provide a PR, but I'm not sure in what style you would like the change to be implemented.

@ForbesLindesay
Copy link
Owner

A third option would be to add the fields property to the array returned by then-mysql.

i.e. the then-mysql code would look something like:

const results = [...results];
results.fields = fields;
return fields;

I would accept any of the three proposals, please select whichever you feel provides the best developer experience, keeping in mind that the most important use case is people who just want the results of the query, and do not need the added metadata.

@ZaubererMD
Copy link
Author

Neat, I did not think of that one. I am pretty new to the Node.js ecosystem, if I change anything do I have to update any meta-files or will the sourcecode be enough? Also, after incorporating the change in then-mysql, do I have to refer to the new version of it somewhere when doing the changes in sync-mysql?

@ForbesLindesay
Copy link
Owner

If you make the change to then-mysql, it will automatically be picked up by this package. The only file you should need to edit is https://github.com/then/then-mysql/blob/master/lib/connection.js

@ZaubererMD
Copy link
Author

ZaubererMD commented Feb 8, 2019

When adding the fields property to the rows-array as you proposed, it somehow is treated as an element of the array and gets assigned an index, as if I had added it with rows.push(fields);. I really can't find any helping source on why this would happen.

My change in https://github.com/then/then-mysql/blob/master/lib/connection.js was to add the line rows.fields = fields; just before the resolve-statement on line 47.

I cannot reproduce this behaviour for properties of arrays anywhere else, not in browsers and not in Node.js other than this instance.
In general I would like to take this approach because I consider it the most elegant of the three solutions.

Do you have any idea what could cause this?

Edit: Of course I also added the fields parameter to the anonymous function used as a callback on line 45.

@ForbesLindesay
Copy link
Owner

Can you submit a pull request with your proposed changes. It's really hard to interpret what change you've made from this.

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

2 participants