-
Notifications
You must be signed in to change notification settings - Fork 1.3k
fix: refactor mcp servers into separate packages #9061
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
|
Build successful! 🎉 |
snowystinger
left a comment
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.
I'm sure I just missed some conversation but, why split it in two? I thought having 2 different api end points was good enough?
packages/dev/mcp/package.json
Outdated
| "name": "mcp-packages", | ||
| "private": true, |
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.
why not have these be shared? is the code just inlined when bundled right now such that you don't need a shared package?
the project structure is a bit odd as a result
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.
Where would the dependencies go without a shared package? I guess one option is to have the shared code in one of the two packages, but it seemed better to have an internal shared one.
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.
right, but the structure is weird as it is right now
- this package is private
- this package is inside of a 'src' directory
- modules imported from this package are referenced via a relative path instead of from the package workspace name
this is why i asked if the code was being inlined instead of actually shared
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.
I've improved the structure to avoid confusion. I think this is what we want:
mcp-sharedis a private package@react-spectrum/mcpand@react-aria/mcpimport from there using relative paths
I'm not sure about outputing src/ in the dist folder. Would that be a tsconfig.json setting?
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.
is a private package
import from there using relative paths
why?
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.
I don't think these packages are bundled right now, just compiled with tsc.
|
Build successful! 🎉 |
snowystinger
left a comment
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.
structure is still weird to me, but i'd prefer we move this along and we can figure out the private vs public shared package later
|
Build successful! 🎉 |
LFDanLu
left a comment
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 good to me, tested the functionality in cursor and it seems to work well. Noticed a warning when building the mcp package, but might be benign?
| "version": "0.1.0", | ||
| "description": "MCP server for React Spectrum (S2) and React Aria documentation", | ||
| "description": "MCP server for React Spectrum (S2) documentation", | ||
| "type": "module", |
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.
maybe benign but I got this when running yarn workspace @react-spectrum/mcp build
(node:82090) [MODULE_TYPELESS_PACKAGE_JSON] Warning: Module type of file:///Users/danilu/dev/react-spectrum/packages/dev/s2-docs/src/iconAliases.js is not specified and it doesn't parse as CommonJS.
Reparsing as ES module because module syntax was detected. This incurs a performance overhead.
To eliminate this warning, add "type": "module" to /Users/danilu/dev/react-spectrum/packages/dev/s2-docs/package.json.
weird since this does have "type": module...
Splits the mcp servers into separate packages. Include a private internal package for shared code.
Before:
After:
Also updates the npx commands to use
@latest✅ Pull Request Checklist:
📝 Test Instructions:
Generate markdown docs:
Build the MCP servers:
Start a docs server:
Update your MCP config:
{ "mcpServers": { "React Spectrum (S2)": { "command": "node", "args": [ "{your-path}/react-spectrum/packages/dev/mcp/s2/dist/s2/src/index.js" ], "env": { "DOCS_CDN_BASE": "http://localhost:1234" } }, "React Aria": { "command": "node", "args": [ "{your-path}/react-spectrum/packages/dev/mcp/react-aria/dist/react-aria/src/index.js" ], "env": { "DOCS_CDN_BASE": "http://localhost:1234" } } } }🧢 Your Project: