V3 changes

[nfriedly]

Update: PR at #74

---

I'm thinking about the changes I'd like to see in a version 3. The two big ones that I'd like to get in are:

• Automatically run splitCookieString() when when the input has a string for the header value (not an array)
  • Probably with a new option to explicitly always/never run it, and only decide based on input when it's unset
• Offer an ESM version of the library

Things I'm considering but haven't yet decided one way or the other:

• Ship both ESM and CJS variations or make 3.x exclusively ESM?
  • Possibly via https://github.com/wessberg/cjstoesm in a pre-commit hook to build the esm version
• Convert the library to typescript?
  • Run tsc twice for ESM and CJS, although TypeScript seems to break the "naked" require in CJS.
• Split parse() into parseToArray() and parseToMap() to make for simpler typing? (Or support all 3?)
  • I think I won't do this one. I hadn't realized typescript supported overloaded signatures, so I can make the typing work correctly with a single function.
• Options for handling errors / malformed cookies - currently it logs in certain cases and silently deals with it in others. I think I'd like to have a strict mode that throws on invalid cookies, a loose mode that silently does it's best, and maybe a "warn" mode that logs and then does it's best.

[bytesnz]

I reckon for the splitting, just adding an additional function of parseToMap() would be good (and keeping the current parse() as is. I'm sure quite a few people will be currently doing:

const cookies = setCookieParser.parse(setCookieParser.splitCookieString(res.headers.get('set-cookie'))).reduce((obj, c) => { obj[c.name] = c; return obj})

Just adding the parseToMap() instead of replacing parse() with parseToArray() and parseToMap() will mean no changes required when upgrading.

[nfriedly]

There's already a built-in map option that does basically that internally. I was thinking it would be difficult to represent in TypeScript, but it turns out that it's not so hard after all - TypeScript can statically determine the output type based on the input options.

If I was starting over from scratch, I'd probably do like you said, but since the existing API is there and doesn't seem to be causing any trouble, I'm going to avoid an unnecessary breaking change leave it as-is.

[thib3113]

Hello, just a note about CJS/ESM .

yes ESM is great, but shipping only in ESM will force other lib to migrate to ESM to (wich can be a really hard work) ... so, please keep CJS/ESM .

Else, I've done multiple libraries that export CJS/ESM, here is an example, in general I'm using esbuild to build it (because I can create a script to call it, but other options exists) (and if he can compile typescript really fast, he doesn't typecheck, so when I want to typecheck I call tsc --noEmit) .

Here is the little esbuild script : https://github.com/thib3113/node-crowdsec/blob/main/packages/crowdsec-client/esbuild.js

I just setup up some options, and create links file that will reexport another parts (I also generate a pkg.ts to export the current version, but you can remove this) .

and finally some configs in package.json : https://github.com/thib3113/node-crowdsec/blob/main/packages/crowdsec-client/package.json#L5-L8 + https://github.com/thib3113/node-crowdsec/blob/main/packages/crowdsec-client/package.json#L27-L39
( using type = module will change your package to module, so you will need to use import, stop using require, and import file with extension .... but because I use .cjs and .mjs you can decide to don't use module type ) .

And here is the result : https://www.npmjs.com/package/crowdsec-client?activeTab=code

about size, yes using CJS and ESM consume more space ... but, if you use a bundler for your code, it will remove the unused files, so in general will use the ESM files, and remove the cjs . npm show a size of 275 kB, but when it was bundled, bundlephobia return a size of 15kB

[nfriedly]

I might look into that more, thanks.

[realityking]

I'd also like to suggest shipping both ESM and CJS versions of the package to maximize compatibility.

However I'd also encourage you to drop the "naked require" in the CJS version and only offer named exports. This way you'll avoid users running into bugs if they swap between ESM and CJS (e.g. due to bundler settings).

[nfriedly]

What kind of bugs?

Also, I'd rather not drop the naked require option, even in a major version, because it would break literally every existing user who upgrades. Just seems like a dick move, especially when it's fairly straightforward to avoid.

For express-rate-limit, we just append a footer to the CJS version that rewrites the exports to what it should have been all along:

https://github.com/express-rate-limit/express-rate-limit/blob/5b7a8dff73c5321cc4d2c24863a94af441ce692b/package.json#L59

I'll probably do something like that to maintain backwards compatibility, if I do switch this lib over to TS.

[benmccann]

ESM-only has just become a reasonable path forward now that all LTS versions of Node support require(ESM)

https://joyeecheung.github.io/blog/2024/03/18/require-esm-in-node-js/

[nfriedly]

Oh, good point. I'm a little embarrassed at how long I've put off this update for, but maybe that will make it worth the wait.

[thib3113]

using a nodejs version that support require(ESM), doesn't tell all CJS projects can use ESM ...

I got the problem recently on a project, with the jose library . I'm working on an old cjs project, and it's not possible to use ESM only libraries ( I don't remember the exact error, and we are using node 24 )

[nfriedly]

Well, drat.

[benmccann]

You cannot use all ESM libraries in CJS projects with recent versions of Node, but only those that do not use top-level await and specify module-sync in the package.json. I'm guessing the libraries you referred to did not do so, but it's hard to say without more details. Without a more specific reason why it wouldn't work, I still expect this library could take advantage of the new module-sync

[nfriedly]

I have a PR up at #74 now