-
Notifications
You must be signed in to change notification settings - Fork 28
Feature/dual module support #14
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
Conversation
|
@Pederzh @pietrozullo Would appreciate it if you could take a look when you have a moment. Let me know if there's anything you'd like me to change or improve! |
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.
Pull Request Overview
Adds support for CommonJS builds while retaining existing ES module output and reorganizes the project root.
- Introduces a new
tsconfig.cjs.jsonfor CJS compilation intodist-cjs - Updates
package.jsonto wire up"require"exports, switchmainto CJS, and add build scripts - Moves the primary entry file into
src/and removes the old rootindex.ts
Reviewed Changes
Copilot reviewed 4 out of 7 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tsconfig.cjs.json | Add CJS compiler config targeting dist-cjs |
| src/index.ts | New consolidated entry point exporting all public APIs |
| package.json | Update exports, main, add CJS build scripts and files |
| index.ts | Remove root entry file in favor of src/index.ts |
Comments suppressed due to low confidence (2)
tsconfig.cjs.json:10
- The CJS tsconfig still includes a top-level "index.ts" which has been removed. This will prevent the compiler from picking up
src/index.ts. Update the include to refer tosrc/index.tsor remove the rootindex.tsentry.
"include": ["index.ts", "src/**/*.ts"],
package.json:86
- [nitpick] The
lodashdependency appears unused in the new code paths. If you’re not importing it anywhere, removing it will reduce package bloat.
"lodash": "^4.17.21",
package.json
Outdated
| }, | ||
| "scripts": { | ||
| "build": "rm -rf dist && tsc", | ||
| "build:cjs": "rm -rf dist-cjs && tsc --project tsconfig.cjs.json && echo '{\"type\":\"commonjs\"}' > dist-cjs/package.json", |
Copilot
AI
Jul 16, 2025
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.
[nitpick] The build:cjs script uses Unix-only commands (rm, shell redirection) which may break on Windows. Consider using a cross-platform tool like rimraf or a Node-based script to ensure compatibility.
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.
@zkabyken, can you also have a look at this please?
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 @Pederzh! Added a script to support cross-platform CommonJS build support using shx that is then executed from package.json. Also made sure to resolve merge conflicts and double checked npm build runs (:cjs and :all)
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 for the discord link! mcp-use is brilliant and I see it being very widely used soon!
Hi @zkabyken we're glad that you are a fan of the project, thanks for your support :) Thanks for taking the time to create this PR! We'd love to have you join our Discord community to chat about how you're planning to use mcp-use! |
|
Hi @Pederzh, I’ve addressed all the comments, this PR should be fully ready now. Let me know if there’s anything else you’d like me to tweak. Super excited to see it get merged! |
|
Hi @pietrozullo! Wondering if you could take a look at this PR whenever possible, would love to continue contributing after this is merged! |
|
Hi, I really love this project and find it very good and useful. Thanks in advanced. |
Hello guys @zkabyken and @RefaelEX , this PR has been stale for a while because it doesn't provide any tests for the changes, and also certifies the double support. I'd like to invite you to create some tests and update them by merging the latest version into this branch, and then create additional tests to verify that the library will continue to function properly. PS I've recently removed |
Description
Adds CommonJS build support alongside existing ES module support, enabling the library to work in both CommonJS and ES module environments while maintaining full backward compatibility. This enhancement allows the library to be used in a wider range of Node.js environments and build systems
The changes include:
"require"export and updated"main"entry point in package.jsonbuild:cjsandbuild:allscripts for CommonJS compilationtsconfig.cjs.jsonfor CommonJS TypeScript configurationdist-cjs/package.jsonwith"type": "commonjs"for proper module resolutionindex.tstosrc/index.tsfor better code organizationBig fan of the project and would love to see it succeed! The dual module support will make the library more accessible to different development environments and build systems.
Type of change
Checklist