refactor(open-sse): JS → TS migration + v0.8.5 - #62
Conversation
- Rename all 94 .js files to .ts across config/, utils/, services/, handlers/, executors/, translator/, transformer/, and root index - Add proper TypeScript interfaces: RegistryEntry, AudioProvider, FetchOptions - Add class property declarations to BaseExecutor - Type key function parameters in utils layer (tlsClient, stream, logger, error, proxyFetch, proxyDispatcher, etc.) - Add @ts-ignore annotations for remaining TS2339 errors (gradual migration) - Zero TypeScript errors in open-sse/tsconfig.json - Zero .js files remaining in open-sse/
- Remove 209 standalone @ts-ignore annotations - Fix 11 root-cause object literals with Record<string, any> typing - Properly type message objects in responseTranslator, kiro executor - Re-insert 164 targeted @ts-ignore for remaining complex patterns - Net reduction: 231 → 186 @ts-ignore (-20%) - Zero TypeScript errors maintained
- Remove ALL 231 @ts-ignore annotations (231 → 0) - Fix 237 TypeScript errors with proper typings: - Type 30+ variable declarations as Record<string, any> - Add optional params: model?, provider?, retryAfter?, retryAfterHuman? - Replace @ts-ignore with 'as any' casts for custom Error/Array properties - Add EdgeRuntime global declaration for edge runtime compat - Import fs/path in responsesTransformer.ts - Fix function argument mismatches (resolveComboConfig, unavailableResponse) - Build: tsc --noEmit passes with 0 errors
- Update package.json version to 0.8.5 - Update package-lock.json version to 0.8.5 - Update version references in docs/new-features
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
Summary of ChangesHello @diegosouzapw, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request undertakes a substantial refactoring effort to transition the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Pull request overview
This PR migrates 94 JavaScript files to TypeScript in the open-sse package and bumps the version from 0.8.0 to 0.8.5. The migration achieves 0 TypeScript compilation errors without using @ts-ignore comments. However, this is largely accomplished by disabling strict mode and extensively using the any type, which significantly reduces the type safety benefits that TypeScript provides.
Changes:
- Version bump from 0.8.0 to 0.8.5 across package.json and package-lock.json
- Addition of TypeScript configuration (tsconfig.json) with strict mode disabled
- Migration of 94 .js files to .ts with type annotations (many using
any) - Breaking API change to
errorResponsefunction parameter order
Reviewed changes
Copilot reviewed 56 out of 97 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Version bump to 0.8.5 |
| package-lock.json | Lockfile version update to 0.8.5 |
| open-sse/tsconfig.json | New TypeScript configuration with strict mode disabled |
| open-sse/**/*.ts | Migrated files with type annotations, extensive use of any type |
| open-sse/handlers/rerank.ts | Breaking change: errorResponse parameter order reversed |
| open-sse/handlers/moderations.ts | Same errorResponse parameter order change |
| open-sse/handlers/audioTranscription.ts | Same errorResponse parameter order change |
| open-sse/handlers/audioSpeech.ts | Same errorResponse parameter order change |
| open-sse/executors/cursor.ts | Global EdgeRuntime declaration without proper typing |
| open-sse/transformer/responsesTransformer.ts | Unused imports added |
Comments suppressed due to low confidence (4)
open-sse/handlers/rerank.ts:84
- The function signature for
errorResponseappears to have changed parameter order from(message, statusCode)to(statusCode, message)based on the usage changes in rerank.ts, moderations.ts, and audioTranscription.ts. This is a breaking API change that could cause runtime errors if not updated consistently across all callers. Verify all call sites have been updated.
open-sse/transformer/responsesTransformer.ts:2 - Unused imports added at the top of this file. The
fsandpathimports are not used anywhere in the visible code and should be removed.
open-sse/executors/cursor.ts:1 - The global declaration
declare var EdgeRuntime: any;at the top of the file is concerning. This appears to be a workaround for a missing type definition. Consider:
- Installing the proper type definitions package if available
- Creating a proper type definition file (e.g.,
edge-runtime.d.ts) - Adding a comment explaining why this declaration is needed
open-sse/handlers/chatCore.ts:55
- Using
@ts-ignoresuccessor patterns like JSDoc@param {any}annotations alongside TypeScript defeats the purpose of migration. These files should either have proper TypeScript types or remain as JavaScript. This pattern appears in multiple files (chatCore.ts, combo.ts, rerank.ts, etc.).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| "compilerOptions": { | ||
| "target": "ES2022", | ||
| "module": "esnext", | ||
| "moduleResolution": "bundler", | ||
| "allowJs": true, | ||
| "checkJs": true, | ||
| "noEmit": true, | ||
| "skipLibCheck": true, | ||
| "esModuleInterop": true, | ||
| "strict": false, | ||
| "jsx": "react-jsx", | ||
| "lib": ["dom", "esnext"], | ||
| "baseUrl": "..", | ||
| "paths": { | ||
| "@/*": ["./src/*"], | ||
| "@omniroute/open-sse": ["./open-sse"], | ||
| "@omniroute/open-sse/*": ["./open-sse/*"] | ||
| } | ||
| }, | ||
| "include": [ | ||
| "**/*.ts", | ||
| "**/*.js" | ||
| ] | ||
| } |
There was a problem hiding this comment.
The any type is used extensively throughout this migration (over 100+ occurrences in the diff), which defeats the purpose of TypeScript's type safety. Consider defining proper interfaces or types instead of using any, especially for:
- Function parameters (e.g.,
options: any,context: any,credentials: any) - Object properties (e.g.,
Record<string, any>) - Type assertions (e.g.,
as any)
While this may require more upfront work, it will provide actual type safety benefits and catch potential bugs at compile time.
| "noEmit": true, | ||
| "skipLibCheck": true, | ||
| "esModuleInterop": true, | ||
| "strict": false, |
There was a problem hiding this comment.
strict: false in tsconfig.json disables all strict type checking options. This significantly reduces the benefits of TypeScript migration. Consider enabling strict mode gradually by enabling individual strict flags (strictNullChecks, strictFunctionTypes, etc.) to improve type safety.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request primarily focuses on enhancing type safety across the codebase by introducing explicit TypeScript interfaces and type annotations to various functions, variables, and object properties. Key changes include defining interfaces for AudioModel, AudioProvider, RegistryModel, RegistryOAuth, and RegistryEntry to provide structured typing for configuration objects. Many functions, such as those in audioRegistry.ts, providerRegistry.ts, and various handlers and services files, received explicit parameter and return type annotations. The tsconfig.json file was also added to enable TypeScript compilation. Review comments highlight concerns about the widespread use of any types, suggesting that while it aids migration, it undermines type safety. Specific feedback recommends defining custom error classes instead of casting properties to any on generic Error objects, using type guards for FormDataEntryValue to safely access properties, and defining proper interfaces for function parameters and return types to avoid any and improve code maintainability and readability. There's also a suggestion to enable strict mode in TypeScript for better error detection.
| "noEmit": true, | ||
| "skipLibCheck": true, | ||
| "esModuleInterop": true, | ||
| "strict": false, |
There was a problem hiding this comment.
Setting "strict": false significantly reduces the benefits of using TypeScript. While it can make the initial migration easier, the long-term goal should be to enable strict mode ("strict": true) to catch a wide range of potential errors at compile time, such as implicit any types, null/undefined handling, and more. I recommend creating a follow-up task to enable strict mode and fix the resulting type errors.
| * @returns {number} score 0 = unhealthy, 100 = perfectly healthy | ||
| */ | ||
| export function getAccountHealth(account) { | ||
| export function getAccountHealth(account, model?: any) { |
There was a problem hiding this comment.
The model parameter was added to the function signature but is not used within the function body. If it's intended for future use, consider adding a comment explaining why. Otherwise, it should be removed to avoid confusion. Also, using any should be avoided; if the model parameter is needed, it should have a proper type.
|
|
||
| try { | ||
| const response = http2 | ||
| const response: any = http2 |
There was a problem hiding this comment.
Using any for the response variable defeats the purpose of TypeScript. It seems makeHttp2Request and makeFetchRequest return a similar object structure. It would be beneficial to define an interface for this response shape and use it here to ensure type safety.
For example:
interface CustomResponse {
status: number;
headers: Record<string, string | number | string[]>;
body: Buffer;
}
// ...
const response: CustomResponse = http2
? await this.makeHttp2Request(url, headers, transformedBody, signal)
: await this.makeFetchRequest(url, headers, transformedBody, signal);| // Default: OpenAI/Groq-compatible multipart proxy | ||
| const upstreamForm = new FormData(); | ||
| upstreamForm.append("file", file, file.name || "audio.wav"); | ||
| upstreamForm.append("file", /** @type {Blob} */ (file), /** @type {any} */ (file).name || "audio.wav"); |
There was a problem hiding this comment.
Using /** @type {any} */ to access the .name property bypasses type checking. Since file is of type FormDataEntryValue (string | File), a type guard would be safer to ensure you only access .name when file is a File object.
| upstreamForm.append("file", /** @type {Blob} */ (file), /** @type {any} */ (file).name || "audio.wav"); | |
| upstreamForm.append("file", /** @type {Blob} */ (file), file instanceof File ? (file.name || "audio.wav") : "audio.wav"); |
| const val = formData.get(key); | ||
| if (val !== null && val !== undefined) { | ||
| upstreamForm.append(key, val); | ||
| upstreamForm.append(key, /** @type {string} */ (val)); |
There was a problem hiding this comment.
The val from formData.get(key) can be a File object, not just a string. Casting to string is not type-safe. FormData.append can handle Blob (which File is) or string values, so you can pass val directly without a cast after checking for null/undefined.
| upstreamForm.append(key, /** @type {string} */ (val)); | |
| upstreamForm.append(key, val); |
| const err = new Error(`Semaphore timeout after ${timeoutMs}ms for ${modelStr}`); | ||
| err.code = "SEMAPHORE_TIMEOUT"; | ||
| (err as any).code = "SEMAPHORE_TIMEOUT"; |
There was a problem hiding this comment.
Adding properties to a generic Error object using as any is not type-safe. A better approach is to create a custom error class that includes the code property. This makes error handling more robust and predictable.
class SemaphoreTimeoutError extends Error {
code = "SEMAPHORE_TIMEOUT";
}
const err = new SemaphoreTimeoutError(`Semaphore timeout after ${timeoutMs}ms for ${modelStr}`);| ]; | ||
|
|
||
| for (const [modelKey, info] of Object.entries(data.models)) { | ||
| for (const [modelKey, info] of Object.entries(data.models) as [string, any][]) { |
There was a problem hiding this comment.
Casting to [string, any][] is a sign that the type of data.models is not well-defined. It would be better to define an interface for the info object to avoid using any and improve type safety when accessing its properties like info.quotaInfo.
| // Attach OpenAI intermediate results for logging | ||
| if (openaiResults && sourceFormat !== FORMATS.OPENAI && targetFormat !== FORMATS.OPENAI) { | ||
| results._openaiIntermediate = openaiResults; | ||
| (results as any)._openaiIntermediate = openaiResults; |
There was a problem hiding this comment.
Attaching a property to an array using as any is not type-safe and can be brittle. A better approach would be to return a structured object that contains both the results and the intermediate data, for example: { results: results, openaiIntermediate: openaiResults }. The caller would then need to be updated to handle this new structure.
| return { | ||
| status: response.status, | ||
| headers: Object.fromEntries(response.headers.entries()), | ||
| headers: Object.fromEntries((response.headers as any).entries()), |
There was a problem hiding this comment.
The as any cast here hides potential type issues. The Headers object is iterable, and Object.fromEntries accepts an iterable of key-value pairs. This should work without any in modern environments. If you are facing type compatibility issues between different environments (Node vs. Edge), you could create a helper function to convert headers to an object in a type-safe way, but simply removing the cast is preferable if it works.
headers: Object.fromEntries(response.headers.entries()),| } | ||
|
|
||
| async function patchedFetch(input, options = {}) { | ||
| async function patchedFetch(input: any, options: any = {}) { |
There was a problem hiding this comment.
The parameters input and options are typed as any. To align with the standard fetch signature, they should be typed as RequestInfo | URL and RequestInit respectively. These types are available because you have "lib": ["dom", ...] in your tsconfig.json.
| async function patchedFetch(input: any, options: any = {}) { | |
| async function patchedFetch(input: RequestInfo | URL, options: RequestInit = {}) { |
Native Claude quota scope is honored end to end (claudeQuota normalizer, modelQuotas separation, effort/context-aware model matching). Maintainer rework: merged the release tip twice, reconciling markAccountUnavailable with the OAuth 401 backoff (#14917: the Claude-scope cooldown feeds resolvedCooldownMs, the backoff still overrides it, and a Claude resetAt is only used when the backoff did not apply) and carrying the claudeQuota/modelQuotas fields into the new quotaCacheState.ts leaf (#14820, type-only import). Validated on the tip: 70 related test files 683 pass; the only reds (chat-cooldown-aware-retry #6, false-terminal-401-quota #1, sse-auth #62-66) fail identically on the pure tip. typecheck:core and ESLint clean. File-size ceiling growth is reconciled in the wave follow-up. Thank you @riez!
Summary
Commits
Verification