improving esm support #102

alexreardon · 2024-05-22T23:09:43Z

adding esm outputs (through "module" field and "exports")
still has cjs support through "main" field
moving from es5 target to es2022 target (will likely need a major for this)
now using esbuild for building
added "engines" to package.json

github-actions · 2024-05-22T23:10:17Z

size-limit report 📦

Path	Size
dist/index.js	0 B (-100% 🔽)
./dist/bind.js	95 B (+100% 🔺)
./dist/bind-all.js	211 B (+100% 🔺)

package.json

alexreardon · 2024-05-23T04:45:59Z

tsconfig.json

  "compilerOptions": {
-    "outDir": "dist",
-    "target": "es5",


shifting from es5 to es2022 (likely will be a major)

Andarist · 2024-05-23T08:04:54Z

package.json

This is likely broken. You mention in the description of the PR that it introduces exports but it has not been done. Even if it would, you are specifying type: "module" in this package.json and yet your "main" points to a CJS file with .js extension. That can't work without an additional package.json "in-between".

Thanks! Not adding the "exports" was an oversight. How is it looking now?

Better, question @Andarist: what changes do you think I need to make? I find it's hard to know what is the right way to make sure things will work for folks

No pressure for you to reply though. I'll keep researching :)

my reading of the node module documentation is that "type": "module" should mean that I can use esm everywhere.

But I don't know if that is correct, or if that is only correct from a particular node version

But I don't know if that is correct, or if that is only correct from a particular node version

it's correct since node 12.

if that is correct, I should be able to drop the cjs build and just have an esm build, and have the "main" field point to that

yes, if you don't want to maintain compatibility with the CJS consumers. It might still be good to use exports in such a case as that limits what can be imported from your package (exports is a strict allowlist for accessible files/specifiers). In fact, with that you don't even need main as exports can substitute it just fine

When you say "cjs" consumers, do you mean folks on node using "cjs"? (require('bind-event-listener')

Would this package going esm prevent require('bind-event-listener') usage?

To take a step back: the intention of this PR is to give ESM consumers a better time. Right now esm consumers were relying on the bundler to wrap the cjs outputs with some lame code to get it working on esm.

Ideally folks using node or bundlers would just be able to have a good time 😊

Looks like I have some reading to do: https://nodejs.org/api/packages.html#dual-commonjses-module-packages

Seems like I might be able to get away with just making this package esm only

Would this package going esm prevent require('bind-event-listener') usage?

Yes. If you are going ESM-only then you make it impossible to require this package. At least until this experimental feature lands: https://nodejs.org/en/blog/announcements/v22-release-announce#support-requireing-synchronous-esm-graphs

package.json

alexreardon · 2024-05-24T02:52:58Z

package.json

  "sideEffects": false,
+  "type": "module",


@Andarist I have gone for the approach outlined in the Node conditional exports documentation.

So the package is listed as a "type: "module", but the when "require" is being used we provide CommonJS files by using the .cjs file extension

I am doing some testing now, but this seems really promising

Andarist · 2024-05-26T08:18:40Z

package.json

+  "exports": {
+    "import": "./dist/esm/index.js",
+    "require": "./dist/cjs/index.cjs",
+    "types": "./dist/esm/index.d.ts"


there are two problems here:

conditions are exports are matched from the top to the bottom. It doesn't make sense to have types last here. This condition should usually be first. TS will still try to match import and require conditions when appropriate and it might never reach types in a manifest like this (it would reach it here as a sibling .d.ts file of your import condition but that's coincidental)

TS will still think that this package can't be required. If you are shipping .cjs file here then you should also consider shipping .d.cts file and create conditions structure like { import: { types, default }, require: { types, default } }

Awesome thanks. Do you know how I can get tsc to generate .d.cts?

You can't 😂 Or rather, it generates its outputs based on your inputs. Your input is a .ts file (not an .cts file) so it won't guess that you want some extra .d.cts file here. The thing to understand here is that TS won't really understand that you are trying to ship a dual package from a single set of sources so it won't help you anyhow to generate multiple outputs based on a single input.

You can either cp && mv your .d.ts or use some tool that handles this for you (tsup or tshy)

I think i've done it! I am now using tsup. It looks like the declaration files for esm and tsc are the same (which is what I was getting caught on before - i thought the cjs ones needed to be formatted differently).

Changes:

now using tsup to build dist (and declaration files)

"exports" now pointing to the correct types for given bundles (might not be strictly needed, but seems helpful)

added @arethetypeswrong/cli to validate that the package is setup correctly

Keen to know what you think @Andarist

bind-event-listener-3.0.0.tgz

Attached the npm pack for reference

bind-event-listener-3.0.0.tgz

Now with tsup workaround: egoist/tsup#953 (comment)

alexreardon · 2024-05-27T01:14:07Z

package.json

+    "check": "yarn check:typescript && yarn check:prettier && yarn check:dist",
+    "check:prettier": "prettier --debug-check $npm_package_config_prettier_target",
+    "check:typescript": "tsc --noEmit && tsc --noEmit --project ./test/tsconfig.json",
+    "check:dist": "attw --pack . --ignore-rules no-resolution",


I had to add --ignore-rules no-resolution as I haven't added a node10 mechanism. I could fix that by adding a "main" field that points to the CJS output - but I am not sure that I should

I have added "main" for now - but not sure if I should keep it

Sorry this PR has changed so much 🤭

Andarist · 2024-05-29T07:44:35Z

tsup.config.ts

+      // https://github.com/egoist/tsup/issues/953#issuecomment-2132576167
+      // ensuring that all local requires in `.cjs` files import from `.cjs` files.
+      // require('./path') → require('./path.cjs') in `.cjs` files
+      name: 'fix-cjs-require',


I'm surprised that u need any kind of fixup here. What would break if this was removed?

We are using bind-event-listener in a plugin for eslint@8 which was failing without this fix 😢

Ah, I see. I'd probably stay away from type: module here since you are creating a dual package anyway. That would fix your issue but it would introduce the mirror issue in .mjs files (it just perhaps would be less surprising there that you need to add extensions in your imports)

Dang. I was keen to make cjs more of an after thought which I could drop. Maybe easiest to go the other way 😭

I mean, since u already have those workarounds in place - it's probably fine the way it is. I only have a vague recollection of some React Native versions having problems with non .js files in main but perhaps u can ignore it. Similarly, IIRC, old CRAs were loading .cjs as files and not javascript sources (as in, they were replacing them with URLs to the generated assets, like they do with .png files and more)

I am thinking of leaving it is as "it is an esm module, but it has some cjs fallbacks when we need it". If it doesn't work out, we can go full esm.

Aside: publishing packages that "just work" is a nightmare right now 😭

Any objections @Andarist to merging in it's current state? My open thought is whether I should update "target" to be "ES6" 🤔

I’d bump it even higher although this lib is so small that it probably doesnt use any es2016+ features anyway ;p

Right now I have target set at es2022

I didn't test it at all but the package.json configuration looks fairly OK to me now

alexreardon · 2024-06-06T23:40:02Z

tsconfig.json

    "esModuleInterop": true,
-    "allowSyntheticDefaultImports": true
+    "skipLibCheck": true,
+    "target": "es2022",


I think i'll update this to be "target": "es6"

moving package to esm

c377866

alexreardon requested a review from Andarist May 22, 2024 23:09

alexreardon commented May 22, 2024

View reviewed changes

package.json Show resolved Hide resolved

alexreardon added 9 commits May 23, 2024 09:39

updating deps to ensure everything is working correctly

c4a247c

updating jest setup

11d5a3d

fixing jest

8b802a2

updating prettier

38d82cc

fixing ts-jest

aae91a7

fixing root entry

0dc8393

also supporting cjs

7eec2f5

updating size limit

e05e14f

updating description

ae32bcd

alexreardon changed the title ~~moving package to esm~~ improving esm support May 23, 2024

alexreardon added 3 commits May 23, 2024 13:05

moving to esbuild for building

4296cca

renaming validate to check

363f2cf

adding engines field

d79e21d

alexreardon commented May 23, 2024

View reviewed changes

Andarist reviewed May 23, 2024

View reviewed changes

adding missing exports

e575563

Andarist reviewed May 23, 2024

View reviewed changes

package.json Outdated Show resolved Hide resolved

Andarist reviewed May 23, 2024

View reviewed changes

package.json Outdated Show resolved Hide resolved

esm only

3785d20

Andarist reviewed May 23, 2024

View reviewed changes

package.json Outdated Show resolved Hide resolved

alexreardon added 6 commits May 23, 2024 20:02

just using exports for now

55821c7

updating syntax

0d98636

esm only

4504ef4

reformatting package.json

6586665

fixing build command

684c6fc

wip

9116dc1

alexreardon added 2 commits May 24, 2024 12:34

using esbuild to generate cjs build

2650c97

fixing package.json

b25f8ab

alexreardon commented May 24, 2024

View reviewed changes

Andarist reviewed May 26, 2024

View reviewed changes

moving to tsup for building. checking output with @arethetypeswrong

9810d69

alexreardon commented May 27, 2024

View reviewed changes

alexreardon added 5 commits May 27, 2024 11:18

adding 'main' for now

f25cb99

updating formatting

d113115

adding todo

c3480f3

adding todo

18b5fbf

fixing type imports

a2f7342

Andarist reviewed May 29, 2024

View reviewed changes

alexreardon commented Jun 6, 2024

View reviewed changes

improving esm support #102

Are you sure you want to change the base?

improving esm support #102

Conversation

alexreardon commented May 22, 2024 • edited Loading

github-actions bot commented May 22, 2024 • edited Loading

size-limit report 📦

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexreardon May 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexreardon May 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexreardon May 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexreardon commented May 22, 2024 •

edited

Loading

github-actions bot commented May 22, 2024 •

edited

Loading

alexreardon May 23, 2024 •

edited

Loading

alexreardon May 24, 2024 •

edited

Loading

alexreardon May 29, 2024 •

edited

Loading