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

new Pool() instanceof Pool is false #1612

Closed
elmigranto opened this issue Apr 6, 2018 · 9 comments
Closed

new Pool() instanceof Pool is false #1612

elmigranto opened this issue Apr 6, 2018 · 9 comments

Comments

@elmigranto
Copy link

elmigranto commented Apr 6, 2018

Code

const assert = require('assert');
const {Pool} = require('pg');
const pg = new Pool();

assert(pg instanceof Pool);

Expected

Should run fine.

Actual

AssertionError [ERR_ASSERTION]: false == true

Versions

$ node -v
v8.11.1

$ npm -v
5.8.0

$ npm ls pg-pool
<project>
└─┬ pg@7.4.1
  └── pg-pool@2.0.3 

I am building a wrapper that accepts either Pool or Connection and want to have listeners for pool's events, but not client's. To work around this issue I am using instanceof with right-hand side of Pool.super_. Not sure if this property is intended to be used that way, or won't change later since docs don't mention its existence.

assert(new Pool() instanceof pg.Pool); // throws
assert(new Pool() instanceof pg.Pool.super_); // passes

I'd propose to fix this by adding and exporting a static method that's something like Pool.new(options) instead of exporting a wrapper function. We can keep super_ on it as a property as well, so change won't be semver-breaking. If this is not feasible given dependency on pg-pool module, we can inherit from there with only difference being Pool.Client.

@elmigranto
Copy link
Author

elmigranto commented Apr 6, 2018

Took a look at index.js, and super_ seems to be there only because util.inherits is used. That must be the reason why instanceof gets confused with pg.Pool, but not EventEmitter:

new Pool() instanceof EventEmitter;  // true
new Pool() instanceof Pool;          // false

Woult it be possible to use class BoundPool extends Pool there and add Client to options inside a constructor?

@elmigranto
Copy link
Author

elmigranto commented Apr 6, 2018

Another work-around might be to check against require('pg-pool') directly, though a bit cleaner for my taste, this has the problem of being undocumented, same as checking against pg.Pool.super_:

const {Pool: createPool} = require('pg');
const Pool = require('pg-pool');

createPool() instanceof Pool; // true

@charmander
Copy link
Collaborator

charmander commented Apr 6, 2018

This will be fixed by #1541 in a new major version.

@alxndrsn
Copy link
Contributor

Still occurring in v7.17.1:

> const pg = require('pg'); pool = new pg.Pool(); pool instanceof pg.Pool
false

@alxndrsn
Copy link
Contributor

Also please note that pool._super no longer exists:

> const pg = require('pg'); pool = new pg.Pool(); pool._super
undefined

@charmander
Copy link
Collaborator

@alxndrsn “in a new major version”, so 8.0.0.

@alxndrsn
Copy link
Contributor

Aha of course, sorry for not getting that 🙂

@brianc
Copy link
Owner

brianc commented Jan 23, 2020

I'm releasing this very soon: #2063

@charmander
Copy link
Collaborator

(fixed in pg 8.0.0)

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

4 participants