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

fix(postgraphile): check pgConfig constructor's function, not its name #992

Merged
merged 5 commits into from
Feb 9, 2019
Merged

fix(postgraphile): check pgConfig constructor's function, not its name #992

merged 5 commits into from
Feb 9, 2019

Conversation

Niicck
Copy link
Contributor

@Niicck Niicck commented Feb 9, 2019

This is a bug that occurs when you plug in a pg.Pool into the postgraphile middleware while using webpack's uglify. The "quacksLikePgPool" check fails in 2 places and the postgraphile middleware never runs.

First, (pgConfig instanceof Pool) at https://github.com/graphile/postgraphile/blob/577c1e6df08ebe47e8880665ce99580cf7d375a7/src/postgraphile/postgraphile.ts#L223 is always false, as far as I can tell. Right now the passing comparison would be (pgConfig instanceof Pool.super_). I included both checks for the sake of future-proofing when node-postgres decides to change how it handles subclasses. brianc/node-postgres#1612 and brianc/node-postgres#1541

Second, the check (constructorName(pgConfig) !== 'Pool' && constructorName(pgConfig) !== 'BoundPool') will always fail when we use the uglify plugin for webpack, because uglify eliminates subclass names. webpack-contrib/uglifyjs-webpack-plugin#269. The Uglify team does not consider this a bug, and recommends comparing constructor functions rather than names.

@anatoly314 writing this.constructor.name === 'Parent' is evil, because you don't test if the constructor of this the same as the of of Parent, you only test if the names are equal. The this.constructor === Parent does not test for the name but for the actual function, and thats the only correct way to do a reliable test.

So instead of comparing constructor names in quacksLikePgPool (constructorName(pgConfig) !== 'Pool' && constructorName(pgConfig) !== 'BoundPool') at https://github.com/graphile/postgraphile/blob/577c1e6df08ebe47e8880665ce99580cf7d375a7/src/postgraphile/postgraphile.ts#L227, I proposed a change to compare the actual constructor function obj.constructor === Pool.super_. Typescript does not recognize that super_ is a property of Pool, so I casted it as an any type.

Basically, this fix will give users the options of plugging in a pg.Pool into the middleware while using uglify.

Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

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

Great explanation. One of the reasons this exists is because people sometimes bring their own version of pg which differs from PostGraphile’s one. Unfortunately these changes lose compatibility with that. It seems to me that if your new check passes then it’s definitely a PG pool, so can you restore the old logic and add your new check at the top with a “return true”? Basically do not change the existing logic (as that may be a breaking change) but add your new logic as a shortcut.

Let me know if I’ve misunderstood (written on my phone).

Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@benjie benjie merged commit 419280d into graphile:master Feb 9, 2019
@benjie
Copy link
Member

benjie commented Feb 9, 2019

(Great contribution - thanks!)

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.

2 participants