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

Ability to force *not* loading native client #1894

Closed
gabegorelick opened this issue May 10, 2019 · 15 comments
Closed

Ability to force *not* loading native client #1894

gabegorelick opened this issue May 10, 2019 · 15 comments

Comments

@gabegorelick
Copy link
Contributor

node-postres supports setting NODE_PG_FORCE_NATIVE to force the loading of the native client, but there's no way to force it to not load the native client:

if (typeof process.env.NODE_PG_FORCE_NATIVE !== 'undefined') {
  module.exports = new PG(require('./native'))
} else {
  module.exports = new PG(Client)

  // lazy require native module...the native module may not have installed
  module.exports.__defineGetter__('native', function () {
    delete module.exports.native
    var native = null
    try {
      native = new PG(require('./native'))
    } catch (err) {
      if (err.code !== 'MODULE_NOT_FOUND') {
        throw err
      }
      /* eslint-disable no-console */
      console.error(err.message)
      /* eslint-enable no-console */
    }
    module.exports.native = native
    return native
  })
}

This can cause spurious warnings about pg-native module not found. See sequelize/sequelize#3781 for an example caused by cloning node-postgres objects.

Potential solution: support a NODE_PG_NO_LOAD_NATIVE environment variable. Setting this would cause the native getter to not be defined. Setting both NODE_PG_NO_LOAD_NATIVE and NODE_PG_FORCE_NATIVE could throw an error.

I'm happy to throw together a PR if there's support for something like this.

@seeden
Copy link

seeden commented May 21, 2019

I see Cannot find module 'pg-native' in my logs. This will help

@rasilva-mission
Copy link

Does anyone know how to run pg without 'pg-native'?
ERROR in ./node_modules/pg/lib/native/client.js Module not found: Error: Can't resolve 'pg-native' in 'C:\Users\xxxxxx\Documents\projects\xxxxxx\node_modules\pg\lib\native'

This project compiles electron windows app and I don't want to include the native module just need client.

@imjordanm
Copy link

I am looking for this same solution. All I need is a way of not forcing pg-native, which I can't use because of its libpq dependency causing build failures. Like OP said, if there was an environment variable to bypass the above lazy require module, that would be a life-saver. The last version that works without pg-native is 3.6.4, but that has it's own issues for me with a missing helper.js file.

@charmander
Copy link
Collaborator

@imjordanm Last version of what? pg doesn’t require pg-native.

@imjordanm
Copy link

imjordanm commented Oct 17, 2019

It does, if you look at the code OP posted there is no way of getting around require("./native") which in turn requires pg-native.
This is the error code unless you install pg-native:

ModuleNotFoundError: Module not found: Error: Can't resolve 'pg-native' in '/mnt/d/Backup/Jordan/Jordan/Web/projects/gamifynow/node_modules/pg/lib/native'

Would only need a config option native: true/false, or an environment variable to not force pg-native.

The issue for me is that pg-native has libpq as a dependency and I need to apt-get libpq-dev in order for it to work, which I can't when hosting on Netlify. So I can't use pg-native.

@charmander
Copy link
Collaborator

It does, if you look at the code OP posted there is no way of getting around require("./native") which in turn calls pg-native.

if (typeof process.env.NODE_PG_FORCE_NATIVE !== 'undefined') {
  // lazy require native module...the native module may not have installed
  module.exports.__defineGetter__('native', function () {

If you don’t define the NODE_PG_FORCE_NATIVE environment variable and don’t access pg.native, require("./native") will never be called.

If you’re using some kind of bundler that looks at requires statically, there’s typically a way to add an exception.

@imjordanm
Copy link

imjordanm commented Oct 17, 2019

Thanks for your quick response. I have tried adding and removing the env variable, and all the imports I can think of. Is there some way of not accessing pg.native? I am using create-react-app/webpack. I tried installing react-app-rewired and customize-cra to try ignore native but couldn't get it to work either.

new webpack.IgnorePlugin(/.\/native/)

module.exports = override(babelExclude([path.resolve("pg/lib/native")]));

Perhaps I am not putting the right paths?

@charmander
Copy link
Collaborator

#1187 (comment) suggests

new webpack.IgnorePlugin(/pg-native/, /\/pg\//),

@imjordanm
Copy link

I greatly appreciate your help, it made me go back and have a better look at my webpack configuration. I managed to solve it and will leave what I did here incase someone gets stuck in the same place. This is for people using netlify-lambda alongside node-postgres:

Netlify-lambda has its own webpack config and so the webpack config to exclude the native module needs to be here instead.

//webpack-functions.js
const webpack = require("webpack")

module.exports = {
    plugins: [new webpack.IgnorePlugin(/pg-native/, /\/pg\//)]
  };
//package.json
"build:server": "netlify-lambda build src/functions --config webpack-functions.js"

@gabegorelick
Copy link
Contributor Author

Making native non-enumerable is potentially a better solution than NODE_PG_NO_LOAD_NATIVE, but it's potentially a breaking change.

gabegorelick added a commit to gabegorelick/node-postgres that referenced this issue Oct 17, 2019
Making it non-enumerable means less spurious "Cannot find module"
errors in your logs when iterating over `pg` objects.

`Object.defineProperty` has been available since Node 0.12.

See brianc#1894 (comment)
gabegorelick added a commit to gabegorelick/node-postgres that referenced this issue Oct 17, 2019
Making it non-enumerable means less spurious "Cannot find module"
errors in your logs when iterating over `pg` objects.

`Object.defineProperty` has been available since Node 0.12.

See brianc#1894 (comment)
gabegorelick added a commit to gabegorelick/node-postgres that referenced this issue Oct 17, 2019
Making it non-enumerable means less spurious "Cannot find module"
errors in your logs when iterating over `pg` objects.

`Object.defineProperty` has been available since Node 0.12.

See brianc#1894 (comment)
gabegorelick added a commit to gabegorelick/node-postgres that referenced this issue Oct 17, 2019
Making it non-enumerable means less spurious "Cannot find module"
errors in your logs when iterating over `pg` objects.

`Object.defineProperty` has been available since Node 0.12.

See brianc#1894 (comment)
@papb
Copy link

papb commented Oct 25, 2019

Hello, I am very happy to see this going forward, we in Sequelize have a lot of users complaining about Cannot find module 'pg-native'.

However, my opinion about this (from an outsider perspective, who doesn't really know the internals of what's going on though), is that the existence of this getter itself is a bit strange, no?

Why is this seemingly magical getter necessary? Couldn't it be simply a .getNative() instead?

@gabegorelick
Copy link
Contributor Author

Why is this seemingly magical getter necessary? Couldn't it be simply a .getNative() instead?

That would be a much bigger breaking change. I think simply making native non-enumerable, as proposed in #1992, solves all of the issues that I am aware of without breaking all but the most contrived usages.

@papb
Copy link

papb commented Oct 25, 2019

Hmm, ok, makes sense :)

gabegorelick added a commit to gabegorelick/node-postgres that referenced this issue Dec 6, 2019
Making it non-enumerable means less spurious "Cannot find module"
errors in your logs when iterating over `pg` objects.

`Object.defineProperty` has been available since Node 0.12.

See brianc#1894 (comment)
gabegorelick added a commit to gabegorelick/node-postgres that referenced this issue Dec 6, 2019
Making it non-enumerable means less spurious "Cannot find module"
errors in your logs when iterating over `pg` objects.

`Object.defineProperty` has been available since Node 0.12.

See brianc#1894 (comment)
gabegorelick added a commit to gabegorelick/node-postgres that referenced this issue Jan 3, 2020
Making it non-enumerable means less spurious "Cannot find module"
errors in your logs when iterating over `pg` objects.

`Object.defineProperty` has been available since Node 0.12.

See brianc#1894 (comment)
brianc added a commit that referenced this issue Jan 15, 2020
* Make `native` non-enumerable

Making it non-enumerable means less spurious "Cannot find module"
errors in your logs when iterating over `pg` objects.

`Object.defineProperty` has been available since Node 0.12.

See #1894 (comment)

* Add test for `native` enumeration

Co-authored-by: Gabe Gorelick <gabegorelick@gmail.com>
brianc added a commit that referenced this issue Jan 28, 2020
* Make `native` non-enumerable

Making it non-enumerable means less spurious "Cannot find module"
errors in your logs when iterating over `pg` objects.

`Object.defineProperty` has been available since Node 0.12.

See #1894 (comment)

* Add test for `native` enumeration

Co-authored-by: Gabe Gorelick <gabegorelick@gmail.com>
brianc added a commit that referenced this issue Mar 30, 2020
* Drop support for EOL versions of node (#2062)

* Drop support for EOL versions of node

* Re-add testing for node@8.x

* Revert changes to .travis.yml

* Update packages/pg-pool/package.json

Co-Authored-By: Charmander <~@charmander.me>

Co-authored-by: Charmander <~@charmander.me>

* Remove password from stringified outputs (#2066)

* Remove password from stringified outputs

Theres a security concern where if you're not careful and you include your client or pool instance in console.log or stack traces it might include the database password.  To widen the pit of success I'm making that field non-enumerable.  You can still get at it...it just wont show up "by accident" when you're logging things now.

The backwards compatiblity impact of this is very small, but it is still technically somewhat an API change so...8.0.

* Implement feedback

* Fix more whitespace the autoformatter changed

* Simplify code a bit

* Remove password from stringified outputs (#2070)

* Keep ConnectionParameters’s password property writable

`Client` writes to it when `password` is a function.

* Avoid creating password property on pool options

when it didn’t exist previously.

* Allow password option to be non-enumerable

to avoid breaking uses like `new Pool(existingPool.options)`.

* Make password property definitions consistent

in formatting and configurability.

Co-authored-by: Charmander <~@charmander.me>

* Make `native` non-enumerable (#2065)

* Make `native` non-enumerable

Making it non-enumerable means less spurious "Cannot find module"
errors in your logs when iterating over `pg` objects.

`Object.defineProperty` has been available since Node 0.12.

See #1894 (comment)

* Add test for `native` enumeration

Co-authored-by: Gabe Gorelick <gabegorelick@gmail.com>

* Use class-extends to wrap Pool (#1541)

* Use class-extends to wrap Pool

* Minimize diff

* Test `BoundPool` inheritance

Co-authored-by: Charmander <~@charmander.me>
Co-authored-by: Brian C <brian.m.carlson@gmail.com>

* Continue support for creating a pg.Pool from another instance’s options (#2076)

* Add failing test for creating a `BoundPool` from another instance’s settings

* Continue support for creating a pg.Pool from another instance’s options

by dropping the requirement for the `password` property to be enumerable.

* Use user name as default database when user is non-default (#1679)

Not entirely backwards-compatible.

* Make native client password property consistent with others

i.e. configurable.

* Make notice messages not an instance of Error (#2090)

* Make notice messages not an instance of Error

Slight API cleanup to make a notice instance the same shape as it was, but not be an instance of error.  This is a backwards incompatible change though I expect the impact to be minimal.

Closes #1982

* skip notice test in travis

* Pin node@13.6 for regression in async iterators

* Check and see if node 13.8 is still borked on async iterator

* Yeah, node still has changed edge case behavior on stream

* Emit notice messages on travis

* Revert "Revert "Support additional tls.connect() options (#1996)" (#2010)" (#2113)

This reverts commit 510a273.

* Fix ssl tests (#2116)

* Convert Query to an ES6 class (#2126)

The last missing `new` deprecation warning for pg 8.

Co-authored-by: Charmander <~@charmander.me>
Co-authored-by: Gabe Gorelick <gabegorelick@gmail.com>
Co-authored-by: Natalie Wolfe <natalie@lifewanted.com>
@charmander
Copy link
Collaborator

As of pg 8.0, native isn’t enumerable. (Cloning modules still sounds like a bug in sequelize or in the use of sequelize, though.)

@AWare
Copy link

AWare commented Dec 20, 2022

This seems to really break badly with parcel as a bundler.

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

7 participants