-
Notifications
You must be signed in to change notification settings - Fork 0
Convert the project to use Node.js type stripping #4
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
base: main
Are you sure you want to change the base?
Conversation
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
3628f19
to
7cca8ed
Compare
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
7cca8ed
to
372f99a
Compare
@@ -23,7 +24,7 @@ The `mock-client` directory contains debug clients for testing the Socket MCP se | |||
Direct stdio communication using JSON-RPC protocol: | |||
|
|||
```bash | |||
npm run debug:stdio | |||
npm run debug-stdio |
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 switched to -
instead of :
to disambiguate npm scripts that are run with globs and those just grouped together that don't run together.
|
||
# Build the application | ||
RUN npm run build | ||
COPY . . |
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 so that as the project grows you don't have to constantly update a file list. You are free to change this if you prefer.
CMD [ "node", "--experimental-strip-types", "index.ts" , "--http"] |
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.
Don't need to build in production either!
@@ -1,22 +1,20 @@ | |||
#!/usr/bin/env node | |||
#!/usr/bin/env node --experimental-strip-types |
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.
Had to add a flag to the shebang.
372f99a
to
520d137
Compare
@@ -1,11 +1,11 @@ | |||
#!/usr/bin/env node | |||
import fetch from 'node-fetch'; |
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.
Node has built in fetch now!
@@ -20,9 +20,9 @@ async function parseResponse(response: any) { | |||
|
|||
// Simple HTTP client for testing MCP server in HTTP mode | |||
async function testHTTPMode() { | |||
const baseUrl = (process.env.MCP_URL || 'http://localhost:3000').replace(/\/$/, ''); // Remove trailing slash | |||
const baseUrl = (process.env['MCP_URL'] || 'http://localhost:3000').replace(/\/$/, ''); // Remove trailing slash |
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.
Typescript forced this.
|
||
async function main() { | ||
const serverPath = join(import.meta.dirname, '..', 'index.ts'); |
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.
Lets use __dirname instead of cwd resolutions.
}, | ||
"engines": { | ||
"node": ">=22.0.0", | ||
"npm": ">= 10" |
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.
Added an engines field.
"debug:http": "node ./build/mock-client/http-client.js", | ||
"server:stdio": "SOCKET_API_KEY=${SOCKET_API_KEY} ./build/index.js", | ||
"server:http": "MCP_HTTP_MODE=true SOCKET_API_KEY=${SOCKET_API_KEY} ./build/index.js" | ||
"prepublishOnly": "npm run build", |
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.
When you publish, this will auto-build.
"server:stdio": "SOCKET_API_KEY=${SOCKET_API_KEY} ./build/index.js", | ||
"server:http": "MCP_HTTP_MODE=true SOCKET_API_KEY=${SOCKET_API_KEY} ./build/index.js" | ||
"prepublishOnly": "npm run build", | ||
"postpublish": "npm run clean", |
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.
When the publish is over, it cleans up.
"server:http": "MCP_HTTP_MODE=true SOCKET_API_KEY=${SOCKET_API_KEY} ./build/index.js" | ||
"prepublishOnly": "npm run build", | ||
"postpublish": "npm run clean", | ||
"test": "run-s test:*", |
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.
Added the test lifecycle, with type checking and the node test runner with coverage.
package.json
Outdated
"build": "npm run clean && run-s build:*", | ||
"build:types": "tsc -p tsconfig.declaration.json", | ||
"build:permissions": "chmod +x ./index.js && (chmod +x ./mock-client/*.js 2>/dev/null || true)", | ||
"build-dtx": "run-s build build-dtx:dtx-pack", |
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 kept the build-dtx command out of the main build step, because it seems like something that is done by hand?
520d137
to
fc81cfb
Compare
"index.d.ts", | ||
"index.d.ts.map", | ||
"mock-client/**/*.js", | ||
"mock-client/**/*.d.ts*" |
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.
Alternatively we could just leave everything in a src folder.
@@ -0,0 +1,171 @@ | |||
#!/bin/bash |
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 cleans up declarations and emitted .js files. This keeps your working directory clean of any publish time assets.
@@ -0,0 +1,64 @@ | |||
#!/usr/bin/env node |
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 ported the stdout debug script to a test environment.
@@ -0,0 +1,16 @@ | |||
{ |
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 used during publishing only.
Followup: #5 |
Kind of a bigish change, but in general should simplify the workflow by removing the dev-time build step. No reason not to start using this flow sooner than later.
Everything runs directly with
node
on Node 23 or later ornode --experimental-strip-types
on Node 22. The flag is getting dropped soon as well. All the scripts and shebangs have been updated to include this as well. It's a noop in Node 23 or later.npm run clean
which cleans out the declaration files and js files.:
for-
when not usingrun-s
groupingsIll add in-line comments as well.