Requiring native bindings polutes 'global' #984
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was some nasty global-ish variable reference updating happening when the native module 'initializes' after its require with `require('pg').native`
This fixes the issue by making sure both `require('pg')` and `require('pg').native` each initialize their own context in isolation and no weird global-ish references are used & subsequently stomped on.
| //dictionary of all key:pool pairs | ||
| all: {}, | ||
| //reference to the client constructor - can override in tests or for require('pg').native | ||
| Client: require('./client'), |
There was a problem hiding this comment.
I was 'punching over' this which is bad since this is a singleton. I made it not a singleton since there's no reason for it to be one, and I pass the proper constructor (native or JS) when its initialized.
|
fixed - pushing new patch version to npm now |
|
@brianc This change is breaking the newrelic instrumentation. We used to be able to wrap the client instance used by the pool, and now it's not accessible. Can we expose the client constructor on pools again? Or, can you please advise how to get around this? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was some nasty global-ish variable reference updating happening when the native module 'initializes' after its require with
require('pg').nativeThis fixes the issue by making sure both
require('pg')andrequire('pg').nativeeach initialize their own context in isolation and no weird global-ish references are used & subsequently stomped on.