Ship CommonJS build for librosa-wasm (0.1.3)#10
Merged
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e35c06e63c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
`@olilarkin/librosa-wasm` now ships a CommonJS/Node build alongside the ESM one, so any `require()` consumer (Electron main, plain Node, Ableton Extension Host) gets a working module with no manual conversion. - scripts/make-cjs.mjs: a pure-JS post-build step (no Emscripten needed) that converts the emscripten ESM glue to a `.cjs`, emits a CommonJS wrapper mirroring the compiled ESM entry, and generates a callable `export =` .d.cts (with the type list derived from types.d.ts). The ESM->CJS conversion handles both emscripten createRequire shim forms (static 3.1.x and inline 4.0.x) and asserts no ESM construct survives, failing loudly if the glue ever changes. - scripts/smoke.cjs: a node:test smoke test that require()s the built CJS entry, instantiates the wasm, and computes. - package.json: import/require exports conditions (each with its own types), main -> .cjs, module -> .js, build:cjs/test:cjs wired into build/test, version bumped to 0.1.3. - README: documents require() usage. Verified: 32 ESM tests + 2 CJS smoke tests pass; CJS and ESM consumers type-check under NodeNext; `npm pack` includes the cjs/d.cts/wasm assets.
The 3.1.61 pin dated from the initial commit with no specific reason. Move all three workflows (ci, npm-publish, web-pages) to 4.0.22, the version the WASM package and its test suite were validated against locally.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Makes
@olilarkin/librosa-wasmship a CommonJS/Node build alongside the existing ESM one, so anyrequire()consumer (Electron main, plain Node, Ableton Extension Host) gets a working module with no manual conversion.Two commits: the CommonJS build, and a separate Emscripten version bump.
CommonJS build
scripts/make-cjs.mjs— scripted post-build step (pure JS, no Emscripten needed). It:dist/wasm/librosa_wasm.cjs. The conversion handles both emscriptencreateRequireshim forms (static top-levelimportin 3.1.x, inlineawait importin 4.0.x), rewritesimport.meta.urlandexport default, and then asserts no ESM construct survives — so a future emscripten upgrade fails the build loudly instead of producing a broken.cjs.dist/src/index.cjsby transforming the compiled ESM entry, so the arity table stays single-sourced fromsrc/index.ts.dist/src/index.d.ctsas a callableexport = createLibrosawith a merged namespace re-exporting the function (anddefault) plus all type names (derived fromtypes.d.ts). This matches themodule.exports = createLibrosaruntime soimport x = require(pkg)is callable under node16/nodenext.scripts/smoke.cjs—node:testsmoke test thatrequire()s the built CJS entry, instantiates the wasm, and computes.package.json—import/requireconditions inexports(each with its owntypes);main→.cjs,module→.js;build:cjs/test:cjswired intobuild/test. Version bumped to 0.1.3.README.md— documentsrequire()usage.The existing CI build/test steps already run
npm run buildandnpm test, both of which now include the CJS conversion + smoke test, so no extra publish-workflow wiring was needed.Emscripten bump (separate commit)
ci,npm-publish,web-pages) from3.1.61→4.0.22, the version the package + test suite were validated against locally. The conversion script is version-robust regardless, so this is a toolchain refresh rather than a fix.Verification
module: NodeNext(incl.import x = require(pkg)callable).require("@olilarkin/librosa-wasm")resolves throughexportsto the.cjs, loads the sibling wasm, and computes; ESMimportpath still works.npm packincludesindex.cjs,index.d.cts,librosa_wasm.cjs, and the.wasm.Once published, the downstream extension can drop its vendored
.cjsand consume the package's CJS entry directly.