-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Tweak strict types setup, fix type errors under strictTypes.ts #806
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
Merged
+297
−134
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,10 @@ const strictTypesPath = join(__dirname, '../src/strictTypes.ts'); | |
|
||
let content = readFileSync(typesPath, 'utf-8'); | ||
|
||
// Remove the @deprecated comment block | ||
const deprecatedCommentPattern = /\/\*\*\s*\n\s*\*\s*@deprecated[\s\S]*?\*\/\s*\n/; | ||
content = content.replace(deprecatedCommentPattern, ''); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this omits the "passthrough will be deprecated" comment that's at the top of types.js |
||
|
||
// Add header comment | ||
const header = `/** | ||
* Types remove unknown | ||
|
@@ -24,37 +28,40 @@ const header = `/** | |
|
||
`; | ||
|
||
// Replace all .passthrough() with .strip() | ||
content = content.replace(/\.passthrough\(\)/g, '.strip()'); | ||
// Replace all .passthrough() with a temporary marker | ||
content = content.replace(/\.passthrough\(\)/g, '.__TEMP_MARKED_FOR_REMOVAL__()'); | ||
|
||
// Special handling for experimental and capabilities that should remain open | ||
// These are explicitly designed to be extensible | ||
const patternsToKeepOpen = [ | ||
// Keep experimental fields open as they're meant for extensions | ||
/experimental: z\.optional\(z\.object\(\{\}\)\.strip\(\)\)/g, | ||
/experimental: z\.optional\(z\.object\(\{\}\)\.__TEMP_MARKED_FOR_REMOVAL__\(\)\)/g, | ||
// Keep _meta fields open as they're meant for arbitrary metadata | ||
/_meta: z\.optional\(z\.object\(\{\}\)\.strip\(\)\)/g, | ||
/_meta: z\.optional\(z\.object\(\{\}\)\.__TEMP_MARKED_FOR_REMOVAL__\(\)\)/g, | ||
// Keep JSON Schema properties open as they can have arbitrary fields | ||
/properties: z\.optional\(z\.object\(\{\}\)\.strip\(\)\)/g, | ||
/properties: z\.optional\(z\.object\(\{\}\)\.__TEMP_MARKED_FOR_REMOVAL__\(\)\)/g, | ||
// Keep BaseRequestParamsSchema passthrough for JSON-RPC param compatibility | ||
/const BaseRequestParamsSchema = z\s*\n\s*\.object\([\s\S]*?\)\s*\n\s*\.strip\(\)/g, | ||
/const BaseRequestParamsSchema = z\s*\n\s*\.object\([\s\S]*?\)\s*\n\s*\.__TEMP_MARKED_FOR_REMOVAL__\(\)/g, | ||
// Keep BaseNotificationParamsSchema passthrough for JSON-RPC param compatibility | ||
/const BaseNotificationParamsSchema = z\s*\n\s*\.object\([\s\S]*?\)\s*\n\s*\.strip\(\)/g, | ||
/const BaseNotificationParamsSchema = z\s*\n\s*\.object\([\s\S]*?\)\s*\n\s*\.__TEMP_MARKED_FOR_REMOVAL__\(\)/g, | ||
// Keep RequestMetaSchema passthrough for extensibility | ||
/const RequestMetaSchema = z\s*\n\s*\.object\([\s\S]*?\)\s*\n\s*\.strip\(\)/g, | ||
/const RequestMetaSchema = z\s*\n\s*\.object\([\s\S]*?\)\s*\n\s*\.__TEMP_MARKED_FOR_REMOVAL__\(\)/g, | ||
// Keep structuredContent passthrough for tool-specific output | ||
/structuredContent: z\.object\(\{\}\)\.strip\(\)\.optional\(\)/g, | ||
/structuredContent: z\.object\(\{\}\)\.__TEMP_MARKED_FOR_REMOVAL__\(\)\.optional\(\)/g, | ||
// Keep metadata passthrough for provider-specific data in sampling | ||
/metadata: z\.optional\(z\.object\(\{\}\)\.strip\(\)\)/g, | ||
/metadata: z\.optional\(z\.object\(\{\}\)\.__TEMP_MARKED_FOR_REMOVAL__\(\)\)/g, | ||
]; | ||
|
||
// Revert strip back to passthrough for these special cases | ||
// Revert marker back to passthrough for these special cases | ||
patternsToKeepOpen.forEach(pattern => { | ||
content = content.replace(pattern, (match) => | ||
match.replace('.strip()', '.passthrough()') | ||
match.replace('.__TEMP_MARKED_FOR_REMOVAL__()', '.passthrough()') | ||
); | ||
}); | ||
|
||
// Remove the temporary marker from all remaining locations (these become no modifier) | ||
content = content.replace(/\.__TEMP_MARKED_FOR_REMOVAL__\(\)/g, ''); | ||
|
||
// Add a comment explaining the difference | ||
const explanation = ` | ||
/** | ||
|
@@ -68,7 +75,7 @@ const explanation = ` | |
* - structuredContent: Tool-specific output that can have arbitrary fields | ||
* - metadata: Provider-specific metadata in sampling requests | ||
* | ||
* All other objects use .strip() to remove unknown properties while | ||
* All other objects use default behavior (no modifier) to remove unknown properties while | ||
* maintaining compatibility with extended protocols. | ||
*/ | ||
`; | ||
|
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,93 @@ | ||
#!/usr/bin/env node | ||
|
||
import { execSync } from 'child_process'; | ||
import { readFileSync, writeFileSync, existsSync } from 'fs'; | ||
import { join, dirname } from 'path'; | ||
import { fileURLToPath } from 'url'; | ||
|
||
const __dirname = dirname(fileURLToPath(import.meta.url)); | ||
const rootDir = join(__dirname, '..'); | ||
|
||
const files = [ | ||
'src/client/index.ts', | ||
'src/server/index.ts', | ||
'src/server/mcp.ts' | ||
]; | ||
|
||
console.log('Testing strict types compatibility...'); | ||
console.log('======================================\n'); | ||
|
||
// Backup original files | ||
const backups = {}; | ||
files.forEach(file => { | ||
const fullPath = join(rootDir, file); | ||
if (existsSync(fullPath)) { | ||
backups[file] = readFileSync(fullPath, 'utf8'); | ||
|
||
// Replace imports | ||
const content = backups[file]; | ||
const newContent = content.replace(/from "\.\.\/types\.js"/g, 'from "../strictTypes.js"'); | ||
writeFileSync(fullPath, newContent); | ||
console.log(`✓ Replaced imports in ${file}`); | ||
} | ||
}); | ||
|
||
console.log('\nRunning TypeScript compilation...\n'); | ||
|
||
try { | ||
// Run TypeScript compilation | ||
execSync('npm run build', { cwd: rootDir, stdio: 'pipe' }); | ||
console.log('✓ No type errors found!'); | ||
} catch (error) { | ||
// Extract and format type errors | ||
const output = error.stdout?.toString() || error.stderr?.toString() || ''; | ||
const lines = output.split('\n'); | ||
|
||
const errors = []; | ||
let currentError = null; | ||
|
||
lines.forEach((line, i) => { | ||
if (line.includes('error TS')) { | ||
if (currentError) { | ||
errors.push(currentError); | ||
} | ||
currentError = { | ||
file: line.split('(')[0], | ||
location: line.match(/\((\d+),(\d+)\)/)?.[0] || '', | ||
code: line.match(/error (TS\d+)/)?.[1] || '', | ||
message: line.split(/error TS\d+:/)[1]?.trim() || '', | ||
context: [] | ||
}; | ||
} else if (currentError && line.trim() && !line.startsWith('npm ')) { | ||
currentError.context.push(line); | ||
} | ||
}); | ||
|
||
if (currentError) { | ||
errors.push(currentError); | ||
} | ||
|
||
// Display errors | ||
console.log(`Found ${errors.length} type error(s):\n`); | ||
|
||
errors.forEach((error, index) => { | ||
console.log(`${index + 1}. ${error.file}${error.location}`); | ||
console.log(` Error ${error.code}: ${error.message}`); | ||
if (error.context.length > 0) { | ||
console.log(` Context:`); | ||
error.context.slice(0, 3).forEach(line => { | ||
console.log(` ${line.trim()}`); | ||
}); | ||
} | ||
console.log(''); | ||
}); | ||
} | ||
|
||
// Restore original files | ||
console.log('Restoring original files...'); | ||
Object.entries(backups).forEach(([file, content]) => { | ||
const fullPath = join(rootDir, file); | ||
writeFileSync(fullPath, content); | ||
}); | ||
|
||
console.log('✓ Original files restored.'); |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
#!/bin/bash | ||
|
||
# Script to test strict types by replacing imports and running tests | ||
|
||
echo "Testing strict types compatibility..." | ||
echo "======================================" | ||
|
||
# Save original files | ||
cp src/client/index.ts src/client/index.ts.bak | ||
cp src/server/index.ts src/server/index.ts.bak | ||
cp src/server/mcp.ts src/server/mcp.ts.bak | ||
|
||
# Replace imports | ||
sed -i '' 's/from "\.\.\/types\.js"/from "..\/strictTypes.js"/g' src/client/index.ts | ||
sed -i '' 's/from "\.\.\/types\.js"/from "..\/strictTypes.js"/g' src/server/index.ts | ||
sed -i '' 's/from "\.\.\/types\.js"/from "..\/strictTypes.js"/g' src/server/mcp.ts | ||
|
||
echo "Replaced imports in:" | ||
echo " - src/client/index.ts" | ||
echo " - src/server/index.ts" | ||
echo " - src/server/mcp.ts" | ||
echo "" | ||
|
||
# Run TypeScript compilation to get type errors | ||
echo "Running TypeScript compilation..." | ||
echo "" | ||
npm run build 2>&1 | grep -E "(error TS|src/)" | grep -B1 "error TS" || true | ||
|
||
# Restore original files | ||
mv src/client/index.ts.bak src/client/index.ts | ||
mv src/server/index.ts.bak src/server/index.ts | ||
mv src/server/mcp.ts.bak src/server/mcp.ts | ||
|
||
echo "" | ||
echo "Original files restored." |
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
Oops, something went wrong.
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.
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 temp replaces the imports of "types.js" with "strictTypes.js" and runs all tests