build: add dual package support for ESM and CommonJS#140
build: add dual package support for ESM and CommonJS#140tommyo wants to merge 1 commit intogrimmdude:masterfrom
Conversation
- Update package.json with dual exports configuration (ESM/CJS) - Simplify rollup config to output both module formats - Remove browser-specific build configuration - Update TypeScript config with ES2018 target and ESNext module - Standardize code formatting (spacing in imports/types) - Bump version to 3.1.2 This change enables the package to be consumed by both ESM and CommonJS projects through conditional exports, improving compatibility across different module systems.
grimmdude
left a comment
There was a problem hiding this comment.
Thanks for adding dual ESM/CJS package support — the package.json exports field looks correctly structured. A few issues I'd like addressed before merging:
1. Breaking type change to NoteOnEvent.pitch
NoteOnEvent is a public export, and the pitch type has been narrowed from string|string[]|number|number[] to string|number. External consumers who construct NoteOnEvent directly with array pitches will get a type error after this change. If this narrowing is intentional it should be split into a separate PR with an appropriate version bump and migration note — not bundled into a patch release.
2. Browser/IIFE build silently removed
The entire browser build (browser/midiwriter.js, build/index.browser.js) and @rollup/plugin-replace have been removed. This is a breaking change for anyone consuming the library via <script> tags. The PR description mentions it but this warrants more prominent treatment and a major version bump.
3. Version bump should reflect breaking changes
Given the type narrowing (#1) and browser build removal (#2), a patch bump (3.1.1 → 3.1.2) understates the scope of these changes. This should be at least a minor bump, or major if the browser build removal is considered part of the public API.
4. ESM output missing exports field in Rollup config
The CJS output specifies exports: 'default' but the ESM output has no exports field. Since main.ts uses a default export, this should be explicitly set for consistency and to avoid ambiguity. Please verify the test suite passes against both output formats.
This change enables the package to be consumed by both ESM and CommonJS projects through conditional exports, improving compatibility across different module systems.