Skip to content

Improve Deno compatibility: config-first and safe env access #3547

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.

Sign up for GitHub

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
+46 −26
Open

Improve Deno compatibility: config-first and safe env access #3547

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
150f210
Set default PostgreSQL user to 'postgres' to avoid Deno env access er…
Jobians Sep 19, 2025
39ea164
Prioritize config values over environment variables in ConnectionPara…
Jobians Sep 19, 2025
546e1fd
Wrap NODE_PG_FORCE_NATIVE check in try/catch for Deno compatibility
Jobians Sep 19, 2025
0ba62cd
Make default database user Deno-safe
Jobians Sep 19, 2025
b0c5778
Fixing checks pass
Jobians Sep 19, 2025
43f2c27
Update packages/pg/lib/connection-parameters.js
Jobians Sep 20, 2025
8d7ace8
fix(pg): only guard process.env access when forcing native client
Jobians Sep 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion packages/pg/lib/connection-parameters.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ const defaults = require('./defaults')
const parse = require('pg-connection-string').parse // parses a connection string

const val = function (key, config, envVar) {
if (config[key]) {
return config[key]
}

if (envVar === undefined) {
envVar = process.env['PG' + key.toUpperCase()]
} else if (envVar === false) {
Expand All @@ -15,7 +19,7 @@ const val = function (key, config, envVar) {
envVar = process.env[envVar]
}

return config[key] || envVar || defaults[key]
return envVar || defaults[key]
}

const readSSLConfigFromEnvironment = function () {
Expand Down
9 changes: 8 additions & 1 deletion packages/pg/lib/defaults.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,18 @@
'use strict'

let user
Copy link

@NickCrews NickCrews Oct 26, 2025

Choose a reason for hiding this comment

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

maybe say explicitly let user = undefined? I know they behave the same, but semantically this bare let user (incorrectly) implies to me that we expect to always set it to a value.

try {
user = process.platform === 'win32' ? process.env.USERNAME : process.env.USER
} catch {
// ignore, e.g., Deno without --allow-env
}

module.exports = {
// database host. defaults to localhost
host: 'localhost',

// database user's name
user: process.platform === 'win32' ? process.env.USERNAME : process.env.USER,
user,

// name of database to connect
database: undefined,
Expand Down
57 changes: 33 additions & 24 deletions packages/pg/lib/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,31 +34,40 @@ const PG = function (clientConstructor) {
this.utils = utils
}

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

// lazy require native module...the native module may not have installed
Object.defineProperty(module.exports, 'native', {
configurable: true,
enumerable: false,
get() {
let native = null
try {
native = new PG(require('./native'))
} catch (err) {
if (err.code !== 'MODULE_NOT_FOUND') {
throw err
}
let forceNative = false
try {
forceNative = !!process.env.NODE_PG_FORCE_NATIVE
} catch {
// ignore, e.g., Deno without --allow-env
}

if (forceNative) {
Copy link

@NickCrews NickCrews Oct 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

If I am reading this right, if you are in a deno environment with no --allow-env, then the above will always error, and forceNative will always remain in its default of false, and so it will be impossible to use the native driver. I'm not sure how much of a dealbreaker this actually is (eg does the native driver actually run in deno???).

Copy link
Collaborator

@charmander charmander Oct 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think the environment variable was always a mistake (same with the property), and breaking it in a way that’s backwards-compatible is fine. If a future API of the pg package even includes pg-native at all, I suggest it should be imported from pg/native and that there should be only the one way to do it. (I also recommend never using pg-native, since it has extra bugs and less maintenance. The inherent performance difference is minimal, though both pg and pg-native are both missing key optimizations right now.)

Copy link

@NickCrews NickCrews Oct 28, 2025

Choose a reason for hiding this comment

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

OK, I also agree that the best API would be to import from pg/native. Then we can skip any new APIs here to select the backend. How about this then?

const PG = ... // same as before

function makeClient(config) {
    let useNative = false;
    try {
        useNative = typeof process.env.NODE_PG_FORCE_NATIVE !== 'undefined'
    } catch {
        // ignore, eg in Deno sandbox without --allow-env
    }
    const actualClient = useNative ? require('./native') : Client
    return actualClient(config)
}

module.exports = new PG(makeClient)

// lazy require native module...the native module may not have installed
  Object.defineProperty(module.exports, 'native', {
...

Is there anything else you need to merge this PR? With that change I'm happy. Honestly, the current suggested implementation is similar enough, I would be fine merging that as well.

clientConstructor = require('./native')
}

module.exports = new PG(clientConstructor)

// lazy require native module...the native module may not have installed
Object.defineProperty(module.exports, 'native', {
configurable: true,
enumerable: false,
get() {
let native = null
try {
native = new PG(require('./native'))
} catch (err) {
if (err.code !== 'MODULE_NOT_FOUND') {
throw err
}
}

// overwrite module.exports.native so that getter is never called again
Object.defineProperty(module.exports, 'native', {
value: native,
})
// overwrite module.exports.native so that getter is never called again
Object.defineProperty(module.exports, 'native', {
value: native,
})

return native
},
})
}
return native
},
})