Skip to content

Conversation

@simone-sanfratello
Copy link

No description provided.

Copy link
Member

@delvedor delvedor left a comment

Choose a reason for hiding this comment

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

The test are failing, the native library is slightly different from the standard one.

Please put inside the .gitignore the package-lock.json.

@simone-sanfratello
Copy link
Author

simone-sanfratello commented Oct 14, 2017

Oh, damn. I'll investigate.
Can be an option to create a package "fastify-postgres-native" ?

@delvedor
Copy link
Member

Can be an option to create a package "fastify-postgres-native" ?

Yes, but I prefer keep everything inside here, since the changes to do should be minimal.

@simone-sanfratello
Copy link
Author

updated, it works!

let pg = require('pg')

function fastifyPostgres (fastify, options, next) {
if (options.native) {
Copy link
Member

Choose a reason for hiding this comment

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

Since we are passing the options object to Pool, it's better that we remove the native key from the object.

if (options.native) {
   delete options.native
   ...

@simone-sanfratello
Copy link
Author

ok, done. Please review also the readme, my english need a review :)

Use native `libpq` to gain high performance; it will use [pg-native](https://github.com/brianc/node-pg-native) instead of [pg](https://github.com/brianc/node-pg).
Note: it requires PostgreSQL client libraries & tools installed, see
[instructions](https://github.com/brianc/node-pg-native#install).
Note: trying to use native options without successfully installation of `pg-native` will get a warning and fallback to regular `pg` module.
Copy link
Member

Choose a reason for hiding this comment

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

Since pg-native is not in the devDependencies this warn is not needed.
In my opinion we should have pg-native in our devDependencies, in this way the user can actively choose which library use and not install more stuff than needed.

In this way, I'll rephrase the docs as the following:

### Native option
If you want to gain the maximum performances you can install [pg-native](https://github.com/brianc/node-pg-native), and pass `native: true` to the plugin options.
*Note: it requires PostgreSQL client libraries & tools installed, see [instructions](https://github.com/brianc/node-pg-native#install).*

package.json Outdated
"pg": "^7.3.0",
"pg-native": "^2.2.0"
},
"optionalDependencies": {},
Copy link
Member

Choose a reason for hiding this comment

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

This line can be removed :)

Copy link
Member

@delvedor delvedor left a comment

Choose a reason for hiding this comment

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

LGTM

Well done! :)

@simone-sanfratello
Copy link
Author

Good! Thanks for support!

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@delvedor delvedor merged commit 6299e73 into fastify:greenkeeper/fastify-0.30.1 Oct 16, 2017
@delvedor delvedor mentioned this pull request Oct 16, 2017
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

Successfully merging this pull request may close these issues.

3 participants