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
refactor: tighten up cloudflare detection #3170
base: master
Are you sure you want to change the base?
Conversation
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.
Deploying node-postgres with Cloudflare Pages
|
a6099dd
to
db7d830
Compare
db7d830
to
079a259
Compare
packages/pg/lib/stream.js
Outdated
var tls = require('tls') | ||
if (tls.connect) { | ||
if (process.release && process.release.name === 'node') { | ||
var tls = require('tls') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: if you want extreme pedantry (I mean...who doesn't, right? 😉 ) putting the conditional (process.release && process.release.name === 'node'
) into a predicate function isNode()
or something would DRY this up a bit. I'm also slightly concerned how this impacts bun/deno? Is there any way to invert the check and see if you're inside cloudflare instead? Then if you're not in cloudflare but you're in a fully node compatible environment (be that node, deno, bun) we're using the node side of things. Obviously this is subject to change as the node/deno/bun/cf/browser/wasm environments continue to converge...just my thoughts here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way to invert the check and see if you're inside cloudflare instead?
Yes, that's what I meant by checking if you're in CF (as opposed to checking if you're not in node).
I'm not sure if they expose anything like that. If not, they definitely should add a process.env.CLOUDFLARE_WEB_WORKER
env similar to how GitHub does for actions.
Then if you're not in cloudflare but you're in a fully node compatible environment (be that node, deno, bun) we're using the node side of things.
Reminds me of the io.js days!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course trying to import from cloudflare:sockets
has exactly this behaviour. It is checking whether we are in a Worker or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The import of pg-cloudflare
outside of a worker is an empty package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In BunJS: process.release.name === "node"
but in Deno: process === undefined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also considered a "Cloudflare specific global" such as HtmlRewriter
which doesn't exist in node or deno... but it does exist in bun!
I am asking internally to see if there is a more reliable way to determine the runtime.
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 The approach of trying to import from |
Trying to detect a specific platform based on API presence isn’t an example of that; trying to import |
* | ||
* @returns true if the code is currently running inside a Cloudflare Worker. | ||
*/ | ||
function isCloudflareRuntime() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you memoize this function please? otherwise it would run many times unnecessarily.``
function isCloudflareRuntime() { | |
const isCloudflareRuntime = (function isCloudflareRuntimeInit() { |
} | ||
return false | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | |
})(); |
if (typeof net.Socket === 'function') { | ||
return new net.Socket() | ||
} else { | ||
if (isCloudflareRuntime) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you forgot to call the fn here, but that's ok if you accept my memoization changes below
I kicked off a discussion at Cloudflare about possibly taking a different approach by beefing up our 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 |
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.