refactor: tighten up cloudflare detection

[petebacondarwin]

The previous approach to detecting whether to use Cloudflare's sockets was to check for missing polyfills. But as we improve the polyfills that Wrangler can provide these checks are no longer valid.

Now we just try to use the Cloudflare API first and fallback to Node.js if those are not available.

[cloudflare-workers-and-pages[bot]]

Deploying node-postgres with    Cloudflare Pages

Latest commit:	9003b1f
Status:	✅  Deploy successful!
Preview URL:	https://c599cb34.node-postgres.pages.dev
Branch Preview URL:	https://fix-cloudflare-detection.node-postgres.pages.dev

View logs

[charmander]

I haven’t taken a good look at pg-cloudflare yet, but I think it’s always nicer to detect the APIs that are actually needed than to detect a particular runtime, when possible.

[petebacondarwin]

I haven’t taken a good look at pg-cloudflare yet, but I think it’s always nicer to detect the APIs that are actually needed than to detect a particular runtime, when possible.

That was my original approach way back, where I "knew" that Cloudflare did not support net.Socket. But since we are upgrading our polyfills (and have no control over other people bundling in their own polyfills) we can no longer rely upon that.

The approach of trying to import from cloudfare:sockets is also a similar approach but I accept that having an exception each time for most use cases just to support a minor use case is a bit unfair.

[charmander]

Trying to detect a specific platform based on API presence isn’t an example of that; trying to import cloudflare:sockets is, yeah. Cache the result?

[IgorMinar]

I kicked off a discussion at Cloudflare about possibly taking a different approach by beefing up our node:net#Socket support: https://github.com/cloudflare/workerd/issues/2129

I do think that this PR improves the situation and should be merged, otherwise we'll most likely break pg-cloudflare as we improve our node compatibility which will result in misdetection in the current version of pg-cloudflare. But ideally, we should transition completely away from a need to provide a custom socket within pg as our node:net#Socket should be good enough, but that will take a bit of time.

[petebacondarwin]

Thanks to everyone who has looked at this PR over the weeks it has been evolving. I think it is now in a good enough place to land. Could you take a last look? @brianc , @IgorMinar , @sehrope and @charmander ?

[sehrope]

Having multiple parts of the code handle the caching is odd. Also, that structure makes it more difficult to add other non-native stream implementations.

How about something like this:

function getNodejsStreamFuncs() {
  function getStream(ssl) {
    const net = require('net')
    return new net.Socket()
  }

  function getSecureStream(options) {
    var tls = require('tls')
    return tls.connect(options)
  }
  return {
    getStream,
    getSecureStream,
  }
}

function getCloudflareStreamFuncs() {
  function getStream(ssl) {
    const { CloudflareSocket } = require('pg-cloudflare')
    return new CloudflareSocket(ssl)
  }

  function getSecureStream(options) {
    options.socket.startTls(options)
    return options.socket
  }
  return {
    getStream,
    getSecureStream,
  }
}

/**
 * Are we running in a Cloudflare Worker?
 *
 * @returns true if the code is currently running inside a Cloudflare Worker.
 */
 function isCloudflareRuntime() {
  // Since 2022-03-21 the `global_navigator` compatibility flag is on for Cloudflare Workers
  // which means that `navigator.userAgent` will be defined.
  if (typeof navigator === 'object' && navigator !== null && typeof navigator.userAgent === 'string') {
    return navigator.userAgent === 'Cloudflare-Workers'
  }
  // In case `navigator` or `navigator.userAgent` is not defined then try a more sneaky approach
  if (typeof Response === 'function') {
    const resp = new Response(null, { cf: { thing: true } })
    if (typeof resp.cf === 'object' && resp.cf !== null && resp.cf.thing) {
      return true
    }
  }
  return false
}

function getStreamFuncs() {
  if (isCloudflareRuntime()) {
    return getCloudflareStreamFuncs()
  }
  return getNodejsStreamFuncs()
}

module.exports = getStreamFuncs()

When (if?) we ever change this repo to embed types, that lends itself to a better structure as as each of the getXyzStreamFuncs() function would have the same signature. It also makes it easier to add something in the future as there's a clear pattern and a single place to do so.

---

I haven't tested the above so if you do go with that approach, obviously test it out and make sure it makes sense.

Regarding the check in isCloudflareRuntime(), why is there two different checks? In what situation would the first part not trigger but the second part would? Do they still not have a single env variable to indicate you're running in a Cloudflare worker? (e.g., process.env.CLOUDFLARE_WORKER)

[petebacondarwin]

Thanks for the input @sehrope.

Regarding the check in isCloudflareRuntime(), why is there two different checks? In what situation would the first part not trigger but the second part would? Do they still not have a single env variable to indicate you're running in a Cloudflare worker? (e.g., process.env.CLOUDFLARE_WORKER)

This is because the navigator.userAgent approach does not work if the user has their Cloudflare Workers compatibility date set to older than 2022-03-21.

I did consider a solution similar to the one you propose above. The only downside is that the check for Cloudflare will not happen lazily. But I guess that it is so fundamental to get hold of such a socket that there are very few (if any) scenarios where the library will be used and not ask for a socket. So there is not much value in doing this check lazily.

[brianc]

Hey I just got back from out of town - will take a look at this tomorrow! thanks!

[brianc]

Code looks good to me! I'll get this merged up - thanks. @petebacondarwin are there other things y'all are blocked on wrt cf workers & pg integration? I was talking with @jrf0110 and he said there were some other issues...I'm happy to look at them.

[petebacondarwin]

Hi @brianc - can we run a release of pg to get this change out there? It is needed for the new Cloudflare nodejs compatibility to work properly with this library.