Skip to content

refactor(mcp_service): migrate index.js to TypeScript - #19

Open
shayanshafii wants to merge 1 commit into
masterfrom
devin/1781650131-mcp-service-index-ts
Open

shayanshafii wants to merge 1 commit into
masterfrom
devin/1781650131-mcp-service-index-ts

Conversation

@shayanshafii

@shayanshafii shayanshafii commented Jun 16, 2026

Copy link
Copy Markdown

SUMMARY

Migrates the MCP service Node.js entry module from JavaScript to TypeScript, in line with the repo-wide "NO JavaScript files" modernization guidance in AGENTS.md.

superset/mcp_service/index.js exported a thin SupersetMCPServer wrapper class via CommonJS. It is now index.ts with:

  • Typed constructor options (SupersetMCPServerOptions) and a fully-resolved internal options shape.
  • A typed process: ChildProcess | null field (type imported from child_process).
  • Removal of the previously-unused spawn/path-as-dead-code imports; path is now actually used.

The public CommonJS shape is preserved using export = SupersetMCPServer, so require('@superset/mcp-server') still returns the class directly (compiles to module.exports = SupersetMCPServer).

Because the compiled output now lives in dist/, start() resolves the bin script relative to the package root instead of the source dir:

// before (index.js, run from package root)
require('./bin/superset-mcp.js');
// after (index.ts -> dist/index.js)
require(path.join(__dirname, '..', 'bin', 'superset-mcp.js'));

Tooling added to make the TS source build/run, mirroring the existing superset-websocket Node package convention:

  • tsconfig.json (module: commonjs, target: es2020, strict, declaration: true, outDir: dist).
  • package.json: main -> dist/index.js, added types -> dist/index.d.ts, build/type-check scripts, and typescript + @types/node devDependencies. The bin/superset-mcp.js runner is unchanged and remains the package's bin entry.
  • dist/ and node_modules/ are already covered by the root .gitignore, so no build artifacts are committed.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A — no UI changes.

TESTING INSTRUCTIONS

cd superset/mcp_service
npm install
npm run type-check   # tsc --noEmit, passes
npm run build        # emits dist/index.js + dist/index.d.ts
# smoke test: export is still the class and instantiation works
node -e "const S=require('./dist/index.js'); const s=new S({port:6000}); s.stop(); console.log(S.name, s instanceof S);"
# -> SupersetMCPServer true

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

Link to Devin session: https://app.devin.ai/sessions/5c09535882164e0f9746046ea0776643
Requested by: @shayanshafii


Devin Review

Status Commit
⚪ Not started

Run Devin Review

Open in Devin Review (Staging)
Open in Devin Review

Co-Authored-By: shayan <shayan@cognition.ai>
Copilot AI review requested due to automatic review settings June 16, 2026 22:51
@devin-ai-integration

Copy link
Copy Markdown

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 potential issue.

Open in Devin Review

Comment on lines +67 to 71
start(): void {
// The bin script handles the execution. Resolve it relative to the
// package root so the lookup works from the compiled output directory.
require(path.join(__dirname, '..', 'bin', 'superset-mcp.js'));
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 Pre-existing: stop() is a no-op because start() never sets this.process

The start() method at superset/mcp_service/index.ts:67-71 calls require() to load the bin script, which internally spawns a Python child process. However, that child process reference is local to the bin script (superset/mcp_service/bin/superset-mcp.js:250) and is never exported or returned. As a result, this.process remains null after start(), and stop() at line 73-78 can never actually kill the spawned process. This is unchanged from the original JS — the old code also never set this.process — but it means the class's lifecycle management is incomplete. If anyone ever calls stop(), it silently does nothing.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — agreed, and as the note says this is pre-existing: the original index.js also never set this.process, so stop() has always been a no-op. This PR is intentionally a behavior-preserving JS→TS migration, so I've left the lifecycle logic unchanged rather than expanding scope here. Properly wiring up start()/stop() (e.g. having the bin runner expose the spawned ChildProcess so it can be tracked and killed) would be a good follow-up, but it's a functional change beyond this migration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants