czeidler / knex-pglite

PGlite Dialect for Knex
MIT License
13 stars 2 forks source link

Incorrectly packaged ESM #3

Closed ghchaosfae closed 2 days ago

ghchaosfae commented 6 days ago

Package isn't correctly built for ESM compat.

Can see it shown in the node repl, where you get a result of { default: { default: <class> } }

Welcome to Node.js v22.2.0.
Type ".help" for more information.
> const all = await import("knex-pglite");
undefined
> all
[Module: null prototype] {
  __esModule: true,
  default: { default: [class ClientPGLiteImpl extends Client_PG] }
}
>

In a Node.js ESM module, this ends up being needed for it to work:

import Knex from "knex";
import ClientPGLite from "knex-pglite";

const knex = Knex({
    client: ClientPGLite.default,
    connection: {},
})

We don't see a similar failure in the test because the test is running against the TypeScript source, and TypeScript compiles internally consistently at least. Presumably, importing this module in other similarly compiled TS projects can also work correctly. But it doesn't work correctly in a true ESM environment.

czeidler commented 4 days ago

Thanks for reporting. I guess its because of the weird way I was exporting the class (did it to fix the missing types for the PG client...) I added a declaration file and use a default export now #4.

I haven't tested it with an ESM module though. @ghchaosfae would this fixes it?

ghchaosfae commented 4 days ago

I'm guessing no, because I think it needs the tsconfig adjusted, but,

The best thing to do is directly test it

I tried pulling the repo and building, but npm i + npm run build gets type errors.

Errors ``` $ npm run build > knex-pglite@0.9.7 build > tsc node_modules/@electric-sql/pglite/dist/index.d.ts:4:32 - error TS2307: Cannot find module '@electric-sql/pg-protocol/messages' or its corresponding type declarations. 4 import { BackendMessage } from '@electric-sql/pg-protocol/messages'; ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ node_modules/@electric-sql/pglite/dist/index.d.ts:5:27 - error TS2307: Cannot find module '@electric-sql/pg-protocol/messages' or its corresponding type declarations. 5 import * as messages from '@electric-sql/pg-protocol/messages'; ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ node_modules/@electric-sql/pglite/dist/index.d.ts:7:25 - error TS2307: Cannot find module './messages-CaXcfh5Z.js' or its corresponding type declarations. 7 import { B, a, m } from './messages-CaXcfh5Z.js'; ~~~~~~~~~~~~~~~~~~~~~~~~ node_modules/@electric-sql/pglite/dist/interface-BChI4DHV.d.ts:1:47 - error TS2307: Cannot find module '@electric-sql/pg-protocol/messages' or its corresponding type declarations. 1 import { NoticeMessage, BackendMessage } from '@electric-sql/pg-protocol/messages'; ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ node_modules/@electric-sql/pglite/dist/pglite-Dh_uQEOE.d.ts:3:32 - error TS2307: Cannot find module '@electric-sql/pg-protocol/messages' or its corresponding type declarations. 3 import { BackendMessage } from '@electric-sql/pg-protocol/messages'; ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Found 5 errors in 3 files. Errors Files 3 node_modules/@electric-sql/pglite/dist/index.d.ts:4 1 node_modules/@electric-sql/pglite/dist/interface-BChI4DHV.d.ts:1 1 node_modules/@electric-sql/pglite/dist/pglite-Dh_uQEOE.d.ts:3 ```

If that had worked, the next step would be npm pack, getting a tar build that's equivalent to the npm package.

From there, in a folder I'd do npm init -y, then npm i <path to tar package> and npm i knex.

Add in the code before minus the workaround into test.mjs:

import Knex from "knex";
import ClientPGLite from "knex-pglite";

const knex = Knex({
    client: ClientPGLite,
    connection: {},
})

then run node test.mjs.

Now, in full fairness, that's off the top of my head. If you can offer the corrected build I'll confirm locally I didn't mess up any instructions and see if it works.

Also unfortunately I don't know the correct tsconfig magic off the top of my head to make it "just work" everywhere. tsc is particularly miserable to configure (mainly because TypeScript doesn't actually handle ESM correctly to begin with...).

czeidler commented 3 days ago

Thanks for the explanation. I fixed the build errors by updating the pglite version. I had this problem before but somehow ended up committing a version that was not building correctly...

I tried out node test.mjs as described and can reproduce the problem. However, I haven't figured out how to fix it yet... Do I have to build a version that supports both module types? e.g. build a index.js and a index.mjs? or what is the best way to go forward? Sorry I am new to building modules and haven't found some good docs yet.

ghchaosfae commented 3 days ago

No need to apologize! the problems here are very much not your fault. The ESM and CJS thing is a particularly nasty bit of JavaScript history that we still haven't figured out how to move past.

Playing around with it a bit, this change makes using the module from ESM and CJS with Node work properly.

@@ -5,11 +5,11 @@ import { PGClient } from './pgclient';
 // eslint-disable-next-line @typescript-eslint/no-require-imports
 const Client_PG: typeof PGClient = require('knex/lib/dialects/postgres/index.js');

 export type KnexPGliteConfig = Knex.Config & { connection: { pglite?: PGlite } };

-export default class ClientPGLiteImpl extends Client_PG {
+module.exports = class ClientPGLiteImpl extends Client_PG {
     private pglite;

     constructor(config: KnexPGliteConfig) {
         super({
             ...config,

CJS test:

const Knex = require("knex");
const ClientPGLite = require("knex-pglite");

console.log("ClientPGLite", ClientPGLite);

const knex = Knex({
  client: ClientPGLite,
  connection: {},
});

knex.raw(`select NOW();`).then((result) => {
    console.log(result);
    knex.destroy();
});

ESM test:

import ClientPGLite from "knex-pglite";
import * as all from "knex-pglite";

console.log("ClientPGLite", ClientPGLite);
console.log("all", all);

const knex = Knex({
  client: ClientPGLite,
  connection: {},
});

console.log(await knex.raw(`select NOW();`));

knex.destroy();

However, based on the results of using // @ts-check i think this broke the types for the main export. I'm afraid I've run out of time to work on this though for now, but hopefully that helps.

With this configuration, you are fully making this a CommonJS module, which means it works fine with Node.JS running either CJS or ESM code, because ESM can consume CJS fine. (Soon, CJS will be able to consume synchronous ESM as well, which may make things easier.)

Outputting just CommonJS is the most code compatible thing you can do, and if you can get it to work, I definitely recommend it. But I know that for various projects, the choice they eventually make is exporting both, and including the separate package.json pointers to each. There's some risk of being able to load both unfortunately, which can lead to weird issues (e.g., two separate running PGLite instances). And some projects are only supporting ESM, which doesn't cause many issues for client side code, but many Node.js projects would have to implement workarounds to use it (until they finalize being able to CJS require ESM files anyway, and that update propagates out.)

czeidler commented 2 days ago

Thanks module.exports = ... worked!

And yes you are right why does Typescript does not handled this already? or at least has a compiler option for it, couldn't find a settings that works

I published a new version, let me know if there are other issues.

ghchaosfae commented 2 days ago

@czeidler no problem!

I saw you only bumped the patch version. This is fully a breaking change for your package, since folks using the workaround. Since you're pre-1.0.0, you should bump the minor as that denotes a breaking change.

And if you already published 0.9.9, you might be soon enough to unpublish and push a 0.9.10 that's identical to 0.9.8.

czeidler commented 2 days ago

good point, thanks. I deprecated 0.9.9 and published 0.10.0