-
Notifications
You must be signed in to change notification settings - Fork 7
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
alexreardon
wants to merge
29
commits into
master
Choose a base branch
from
moving-to-esm
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
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 c4a247c
updating deps to ensure everything is working correctly
alexreardon 11d5a3d
updating jest setup
alexreardon 8b802a2
fixing jest
alexreardon 38d82cc
updating prettier
alexreardon aae91a7
fixing ts-jest
alexreardon 0dc8393
fixing root entry
alexreardon 7eec2f5
also supporting cjs
alexreardon e05e14f
updating size limit
alexreardon ae32bcd
updating description
alexreardon 4296cca
moving to esbuild for building
alexreardon 363f2cf
renaming validate to check
alexreardon d79e21d
adding engines field
alexreardon e575563
adding missing exports
alexreardon 3785d20
esm only
alexreardon 55821c7
just using exports for now
alexreardon 0d98636
updating syntax
alexreardon 4504ef4
esm only
alexreardon 6586665
reformatting package.json
alexreardon 684c6fc
fixing build command
alexreardon 9116dc1
wip
alexreardon 2650c97
using esbuild to generate cjs build
alexreardon b25f8ab
fixing package.json
alexreardon 9810d69
moving to tsup for building. checking output with @arethetypeswrong
alexreardon f25cb99
adding 'main' for now
alexreardon d113115
updating formatting
alexreardon c3480f3
adding todo
alexreardon 18b5fbf
adding todo
alexreardon a2f7342
fixing type imports
alexreardon File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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", | ||
|
@@ -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}" | ||
|
@@ -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": { | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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'; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shifting from es5 to es2022 (likely will be a |
||
"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/**/*" | ||
] | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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 specifyingtype: "module"
in thispackage.json
and yet your"main"
points to a CJS file with.js
extension. That can't work without an additionalpackage.json
"in-between".There was a problem hiding this comment.
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?There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's correct since node 12.
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 needmain
asexports
can substitute it just fineThere was a problem hiding this comment.
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?There was a problem hiding this comment.
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 😊
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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