Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improving esm support #102

Open
wants to merge 29 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
c377866
moving package to esm
alexreardon May 22, 2024
c4a247c
updating deps to ensure everything is working correctly
alexreardon May 22, 2024
11d5a3d
updating jest setup
alexreardon May 22, 2024
8b802a2
fixing jest
alexreardon May 22, 2024
38d82cc
updating prettier
alexreardon May 22, 2024
aae91a7
fixing ts-jest
alexreardon May 23, 2024
0dc8393
fixing root entry
alexreardon May 23, 2024
7eec2f5
also supporting cjs
alexreardon May 23, 2024
e05e14f
updating size limit
alexreardon May 23, 2024
ae32bcd
updating description
alexreardon May 23, 2024
4296cca
moving to esbuild for building
alexreardon May 23, 2024
363f2cf
renaming validate to check
alexreardon May 23, 2024
d79e21d
adding engines field
alexreardon May 23, 2024
e575563
adding missing exports
alexreardon May 23, 2024
3785d20
esm only
alexreardon May 23, 2024
55821c7
just using exports for now
alexreardon May 23, 2024
0d98636
updating syntax
alexreardon May 23, 2024
4504ef4
esm only
alexreardon May 23, 2024
6586665
reformatting package.json
alexreardon May 23, 2024
684c6fc
fixing build command
alexreardon May 23, 2024
9116dc1
wip
alexreardon May 24, 2024
2650c97
using esbuild to generate cjs build
alexreardon May 24, 2024
b25f8ab
fixing package.json
alexreardon May 24, 2024
9810d69
moving to tsup for building. checking output with @arethetypeswrong
alexreardon May 27, 2024
f25cb99
adding 'main' for now
alexreardon May 27, 2024
d113115
updating formatting
alexreardon May 27, 2024
c3480f3
adding todo
alexreardon May 27, 2024
18b5fbf
adding todo
alexreardon May 27, 2024
a2f7342
fixing type imports
alexreardon May 29, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions package.json
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner Author

@alexreardon alexreardon May 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😊

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
"name": "bind-event-listener",
"version": "3.0.0",
"private": false,
"type": "module",
"description": "Making binding and unbinding DOM events easier",
"author": "Alex Reardon <[email protected]>",
"license": "MIT",
Expand All @@ -15,8 +16,12 @@
"event listener",
"event handler"
],
"main": "dist/index.js",
"types": "dist/index.d.ts",
"module": "./dist/index.js",
"types": "./dist/index.d.ts",
"exports": {
"./": "./dist/index.js",
"./types": "./dist/index.d.ts"
},
"sideEffects": false,
"config": {
"prettier_target": "src/**/*.{ts,js,jsx,md,json} test/**/*.{ts,js,jsx,md,json}"
Expand All @@ -27,8 +32,8 @@
],
"size-limit": [
{
"path": "dist/index.js",
"limit": "549B"
alexreardon marked this conversation as resolved.
Show resolved Hide resolved
"path": "./dist/index.js",
"limit": "212B"
}
],
"scripts": {
Expand Down
4 changes: 2 additions & 2 deletions src/bind-all.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Binding, InferEventType, Listener, UnbindFn } from './types';
import type { Binding, InferEventType, Listener, UnbindFn } from './types';
import { bind } from './bind';

function toOptions(value?: boolean | AddEventListenerOptions): AddEventListenerOptions | undefined {
Expand Down Expand Up @@ -54,7 +54,7 @@ export function bindAll<
listener: Listener<TTarget, TTypes[K] & string>;
options?: boolean | AddEventListenerOptions;
};
}
},
],
sharedOptions?: boolean | AddEventListenerOptions,
): UnbindFn {
Expand Down
2 changes: 1 addition & 1 deletion src/bind.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { UnbindFn, InferEventType, Binding } from './types';
import type { UnbindFn, InferEventType, Binding } from './types';

export function bind<
TTarget extends EventTarget,
Expand Down
2 changes: 1 addition & 1 deletion src/index.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
export { bind } from './bind';
export { bindAll } from './bind-all';
export { Binding, Listener, UnbindFn } from './types';
export type { Binding, Listener, UnbindFn } from './types';
35 changes: 25 additions & 10 deletions tsconfig.json
Original file line number Diff line number Diff line change
@@ -1,16 +1,31 @@
{
"compileOnSave": true,
"compilerOptions": {
/* Base Options: */
"esModuleInterop": true,
"skipLibCheck": true,
"target": "es2022",
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"allowJs": true,
"moduleDetection": "force",
"isolatedModules": true,
"verbatimModuleSyntax": true,
/* Strictness */
"strict": true,
"noUncheckedIndexedAccess": true,
"noImplicitOverride": true,
/* If transpiling with TypeScript: */
"module": "es2022",
"outDir": "dist",
"target": "es5",
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"module": "commonjs",
"moduleResolution": "node",
/* AND if you're building for a library: */
"declaration": true,
"strict": true,
"forceConsistentCasingInFileNames": true,
"removeComments": true,
"esModuleInterop": true,
"allowSyntheticDefaultImports": true
/* If your code runs in the DOM: */
"lib": [
"es2022",
"dom",
"dom.iterable"
],
},
"include": ["src/**/*"]
}
"include": [
"src/**/*"
]
}
Loading