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

Breaking change in 0.1.7 #19

Closed
twdrake-ap opened this issue Apr 24, 2019 · 13 comments
Closed

Breaking change in 0.1.7 #19

twdrake-ap opened this issue Apr 24, 2019 · 13 comments
Labels
bug Something is broken or not as expected help wanted Great issue to grab, help is appreciated!

Comments

@twdrake-ap
Copy link

knex not initializing correctly, still looking into details

fyi

@cvburgess
Copy link
Owner

damn - im sorry - ill investigate as well - please report anything you find

@cvburgess
Copy link
Owner

what db driver are you using?

@cvburgess
Copy link
Owner

ive tested with PG and SQLite and have no issues... this is probably affecting some users:

https://github.com/cvburgess/SQLDataSource/pull/11/files#diff-bcfcedf7cec98b0d2b60ca67f7881b43R13

I do not have test cases around:

  • mysql
  • mysql2
  • oracledb
  • redshift

And in 0.1.7 they defaulted to the no-unwraping logic... which may be incorrect

@cvburgess cvburgess added bug Something is broken or not as expected help wanted Great issue to grab, help is appreciated! labels Apr 24, 2019
@twdrake-ap
Copy link
Author

We are using PG with knex v0.16.5

@cvburgess
Copy link
Owner

Hmm i dont have that issue with Knex and PG - can you create a code sandbox or similar to help me debug?

@jgoslow
Copy link

jgoslow commented May 1, 2019

I'm also getting this error:

error on getClient TypeError: Cannot read property 'knex' of undefined
at normalizeDBResult (/usr/src/app/node_modules/datasource-sql/SQLCache.js:14:18)
at <anonymous>

Going back to ver 0.1.6 fixed it.

Seems that the changes to the getBatched function there are not passing the this.knex object properly into the normalizeDBResult function.

@a2exfr
Copy link

a2exfr commented May 1, 2019

@Igoslow Hi I think this line should be
return this.loader.load(queryString).then(result => this.normalizeDBResult(result));

Also I found that I need to chage this line
to
switch (this.knex.client.config.client)

but I rewrite it for my needs.

Another important thing for me, is to return parsed result from db, not raw responce from db driver(that is returned now), if this needed feature I can make pr.

@cvburgess
Copy link
Owner

@a2exfr What DB driver are you using?

@a2exfr
Copy link

a2exfr commented May 7, 2019

@cvburgess I use MySQL as the DB. This is knex raw method feature.

@cvburgess
Copy link
Owner

Awesome! Can you open a PR that adds MySQL support - the line code for driver-level editing of the raw response is here:

https://github.com/cvburgess/SQLDataSource/blob/master/SQLCache.js#L22-L23

@cvburgess
Copy link
Owner

@jgoslow i think this may be related to your node version? Can i get more information about your env?

@jgoslow
Copy link

jgoslow commented May 21, 2019 via email

@cvburgess
Copy link
Owner

cvburgess commented Jul 8, 2019

I've published https://github.com/cvburgess/SQLDataSource/releases/tag/v0.2.0 which should fix some of these issues and remove some magic around batching

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is broken or not as expected help wanted Great issue to grab, help is appreciated!
Projects
None yet
Development

No branches or pull requests

4 participants