Making package compatible with ESM

[mitar]

In recent version of node, node is using cjs-module-lexer to try to detect names of exports in CJS modules. It is doing static analysis so it cannot detect everything. Previously this package was using a constructor which throw things off, but I do not there was much benefit of having that, or am I missing something? I simplified it into a simple function and now node can import it as a ESM module. Tested with node v16.0.0. E.g.:

import { Client } from 'pg';

[charmander]

If the name exports is special, this seems like a hack. How about index.mjs and the appropriate package.json exports field?

[mitar]

The whole cjs-module-lexer is a hack/workaround. But it has standard behaviors/structures which are documented and standardized and will stay the same. See the one I am adding in this PR here.

The problem with exports in package.json is that it would require a major semver bump, namely after you add it, it is not possible anymore to import things from inside the package, just at the top level (unless we want to list all files manually there and keep them in sync all the time). See more details here:

Beware: adding an exports map is always a “semver major” breaking change. By default, your users can reach into your package and require() any script they want, even files that you intended to be internal. The exports map ensures that users can only require/import the entry points that you deliberately expose.

I feel this is a rather drastic change. When the one I made works and officially supported by node, while stays backwards compatible.

Let's not make perfect the enemy of the good.

[charmander]

The problem with exports in package.json is that it would require a major semver bump

Seems appropriate enough?

unless we want to list all files manually there and keep them in sync all the time

and this also seems fine if you want it for pg 8. There are few files and there’s nothing it’ll be necessary to keep in sync after a new major version that shouldn’t be kept in sync.

[mitar]

Just to be clear. I do not mind doing it as you are asking. I wanted to make it as simple as possible to make it easier to get it merged. If you are saying that I should do the full thing to get it merged, sure. I just do not want to spend super extra time and then this does not get merged (is one of those PRs which can get hard to keep merge-ready if it is not merged soon after it is made).

So to make it clear:

• You would prefer if I properly make it ESM compatible. My understanding is that we still want to keep it CJS compatible. I can do that (here are some suggestions how).
• Major version bump is OK. Is this something I do in this PR or do you bump yourself after merging?
• Do you want this also on all other packages in this monorepo? I can update all of them in the same way.

[benjamingwynn]

This would be a nice-to-have for us, is this still able to be merged with a major version bump to pg@8?

[marziply]

What's preventing this from being merged in? Seems like a simple feature with ergonomic benefits given the shift from require to import.

[brianc]

What's preventing this from being merged in?

I can't tell if this contains breaking changes or not. Does this release require a semver major bump? Is there any way to write tests for both require and imports style of imports from JS?

[aleclarson]

There's no breaking change here.

All it's doing is allowing cjs-module-lexer (used by Node for CJS compatibility in ESM environments) to statically detect the exports added in the PG constructor. Before this PR, you weren't able to do import { Client } from 'node-postgres' in ESM, only import pg from 'node-postgres'.

I just tested the PR locally and it works in both CJS and ESM environments. I think the chance of a regression in this behavior is incredibly small and thus the benefit of an automated test is negligible.