fix: build pg-cloudflare as a CommonJS module

[mrbbot]

Hey! 👋 We're currently working on an integration between Cloudflare Workers and Vitest, allowing you to run your Vitest tests inside the Workers runtime. A community member tried out an early version of this integration and uncovered an issue with pg/pg-cloudflare (cloudflare/workers-sdk#5127 (comment)).

In pg/lib/stream.js, pg-cloudflare is require()ed...

node-postgres/packages/pg/lib/stream.js

Line 10 in b4bfd63

const { CloudflareSocket } = require('pg-cloudflare')

...but pg-cloudflare is compiled to an ES module. ES modules can't be require()d so this shouldn't work. Fortunately, Workers are usually bundled with esbuild which papers over differences between the module formats and allowed this to work. Our Vitest integration uses a "bundle-less" approach that imports modules at runtime from disk more like Node. This meant pg-cloudflare couldn't be require()d by pg, failing any Workers tests using pg.

This PR updates pg-cloudflare's tsconfig.json to compile to CommonJS instead, fixing this issue. 👍

/cc @petebacondarwin

[petebacondarwin]

LGTM

[petebacondarwin]

Oh dear. CI doesn't agree...

https://github.com/brianc/node-postgres/actions/runs/8256843684/job/22602824382?pr=3168#step:8:1100

[petebacondarwin]

The problem with converting this module to CommonJS is this line:

node-postgres/packages/pg-cloudflare/src/index.ts

Line 40 in 0755342

const { connect } = await import('cloudflare:sockets')

If we compile this code to CommonJS this becomes a dynamic require, which the Wrangler bundler then complains about, since the esbuild output results in a dynamic `require("cloudflare:sockets") call.

Could we solve this instead by marking the package as an ESM module (i.e. "type": "module" in its package.json)?
Surely Vite(st) can handle packages that are declared ESM?

[mrbbot]

Surely Vite(st) can handle packages that are declared ESM?

Not in this case. These files don't get transformed by Vite, and are imported using the workerd module fallback service. This means they have strict module semantics, and require() can't be used to import an ES module. We should be able to keep the dynamic import() in the output rather than converting this to a require(). Not quite sure what the tsconfig.json incantation is though, will investigate more tomorrow. 👍

[mrbbot]

Looks like the magic incantation is "module": "Node16" and "moduleResolution": "Node16". This compiles to a CommonJS module, but keeps dynamic import()s in the output. 👍

[unicoder88]

Hey @petebacondarwin ! I'm also struggling with this issue - node-pg not working under Vitest.

Could we solve this instead by marking the package as an ESM module (i.e. "type": "module" in its package.json)? Surely Vite(st) can handle packages that are declared ESM?

I actually tried this, but then this line fails - https://github.com/brianc/node-postgres/blob/master/packages/pg/lib/stream.js#L41

const { CloudflareSocket } = require('pg-cloudflare')

As a proof of concept, I manually converted compiled node/modules/pg-cloudflare/dist/index.js to CommonJS syntax, and my test finally went through.

const { EventEmitter } = require('events'); // import { EventEmitter } from 'events';

class CloudflareSocket extends EventEmitter {
  // ...
}

module.exports.CloudflareSocket = CloudflareSocket;

Is there any research I should do to help push this through?

[petebacondarwin]

@unicoder88 - are you trying to get the library to work with Vitest in general or using the Cloudflare vitest-pool-workers integration?

[unicoder88]

@petebacondarwin , right, it's specifically Cloudflare vitest-pool-workers

[petebacondarwin]

We have a similar concern with the Vite Environments integration that we are working on.
Hopefully we can resolve this in a general way in the next few weeks.