Skip to content

Commit 730f46d

Browse files
committed
Separate MCP SDK exports into dedicated submodule
1 parent bc96342 commit 730f46d

File tree

6 files changed

+150
-18
lines changed

6 files changed

+150
-18
lines changed

README.md

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -42,21 +42,21 @@ npm install oauth-callback
4242
## Quick Start
4343

4444
```typescript
45-
import {
46-
getAuthCode,
47-
OAuthError,
48-
browserAuth,
49-
fileStore,
50-
} from "oauth-callback";
45+
import { getAuthCode, OAuthError } from "oauth-callback";
5146

5247
// Simple usage
5348
const result = await getAuthCode(
5449
"https://example.com/oauth/authorize?client_id=xxx&redirect_uri=http://localhost:3000/callback",
5550
);
5651
console.log("Authorization code:", result.code);
5752

58-
// MCP SDK integration
53+
// MCP SDK integration - use specific import
54+
import { browserAuth, fileStore } from "oauth-callback/mcp";
5955
const authProvider = browserAuth({ store: fileStore() });
56+
57+
// Or via namespace import
58+
import { mcp } from "oauth-callback";
59+
const authProvider = mcp.browserAuth({ store: mcp.fileStore() });
6060
```
6161

6262
## Usage Examples
@@ -139,7 +139,7 @@ const microsoftAuth = await getAuthCode(
139139
The `browserAuth()` function provides a drop-in OAuth provider for the Model Context Protocol SDK:
140140

141141
```typescript
142-
import { browserAuth, inMemoryStore } from "oauth-callback";
142+
import { browserAuth, inMemoryStore } from "oauth-callback/mcp";
143143
import { Client } from "@modelcontextprotocol/sdk/client/index.js";
144144
import { StreamableHTTPClientTransport } from "@modelcontextprotocol/sdk/client/streamableHttp.js";
145145

@@ -167,7 +167,7 @@ await client.connect(transport);
167167
#### Token Storage Options
168168

169169
```typescript
170-
import { browserAuth, inMemoryStore, fileStore } from "oauth-callback";
170+
import { browserAuth, inMemoryStore, fileStore } from "oauth-callback/mcp";
171171

172172
// Ephemeral storage (tokens lost on restart)
173173
const ephemeralAuth = browserAuth({

examples/notion.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414

1515
import { Client } from "@modelcontextprotocol/sdk/client/index.js";
1616
import { StreamableHTTPClientTransport } from "@modelcontextprotocol/sdk/client/streamableHttp.js";
17-
import { browserAuth, inMemoryStore } from "../src/index";
17+
import { browserAuth, inMemoryStore } from "../src/mcp";
1818

1919
async function main() {
2020
console.log("🚀 Starting OAuth flow example with Notion MCP Server\n");

package.json

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
22
"name": "oauth-callback",
3-
"version": "1.1.0",
3+
"version": "1.2.0",
44
"description": "Lightweight OAuth 2.0 callback handler for Node.js, Deno, and Bun with built-in browser flow, MCP support, and zero dependencies",
55
"keywords": [
66
"oauth",
@@ -57,6 +57,10 @@
5757
"types": "./dist/index.d.ts",
5858
"default": "./dist/index.js"
5959
},
60+
"./mcp": {
61+
"types": "./dist/mcp.d.ts",
62+
"default": "./dist/mcp.js"
63+
},
6064
"./package.json": "./package.json"
6165
},
6266
"dependencies": {
@@ -90,7 +94,7 @@
9094
},
9195
"scripts": {
9296
"build:templates": "bun run templates/build.ts",
93-
"build": "bun run build:templates && bun build ./src/index.ts --outdir=./dist --target=node && tsc --declaration --emitDeclarationOnly --outDir ./dist",
97+
"build": "bun run build:templates && bun build ./src/index.ts --outdir=./dist --target=node && bun build ./src/mcp.ts --outdir=./dist --target=node && tsc --declaration --emitDeclarationOnly --outDir ./dist",
9498
"clean": "rm -rf dist",
9599
"test": "bun test",
96100
"typecheck": "bun tsc -p tsconfig.json --noEmit",

review.md

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,107 @@
1+
# Pull Request #1 Review: Refactor Server with Base Class and Modern Patterns
2+
3+
Hey @7heMech! 👋
4+
5+
First off, thank you so much for taking the time to contribute to this project! I really appreciate your thoughtful improvements to the codebase. Your PR shows a deep understanding of the code structure and brings some excellent modernization ideas. Let me share my detailed review of the changes.
6+
7+
## ✅ What I Love About This PR
8+
9+
### 1. **Base Class Architecture** 🏗️
10+
11+
The introduction of `BaseCallbackServer` is brilliant! This dramatically reduces code duplication across the three runtime implementations (Bun, Deno, Node.js). The shared logic for request handling, timeout management, and cleanup is now centralized, making the codebase much more maintainable.
12+
13+
### 2. **Modern Promise Patterns with Promise.race()**
14+
15+
Your replacement of the manual timeout handling with `Promise.race()` is exactly the kind of modernization this project needed. The old approach with `isResolved` flags and manual `clearTimeout` calls was indeed verbose and error-prone. Your solution is:
16+
17+
- More declarative and readable
18+
- Less prone to race conditions
19+
- Following modern JavaScript best practices
20+
21+
### 3. **Improved Resource Management** 🧹
22+
23+
The `try...finally` block in `waitForCallback` is a significant improvement! This guarantees that listeners are always cleaned up, preventing potential memory leaks. The use of a `Map` for tracking multiple path listeners is also more robust than the single property approach.
24+
25+
### 4. **Better Error Handling for Multiple Listeners** 🛡️
26+
27+
The check for existing listeners on the same path prevents potential conflicts and provides clear error messages. This is a thoughtful addition that improves the developer experience.
28+
29+
## 🔍 Areas That Need Attention
30+
31+
### 1. **TypeScript Type Definitions Issue**
32+
33+
While adding type packages (`@types/deno`, `@types/node`) was a good intention, there's a compilation issue. The TypeScript compiler can't find the Deno global even with the types installed. The PR still requires `@ts-ignore` comments for Deno globals, which somewhat defeats the purpose of adding the type packages.
34+
35+
**Critical issue with Node.js type imports:**
36+
The PR adds `import type { Server as HttpServer } from "node:http"` at the top level, which is problematic for a cross-runtime library:
37+
38+
- These imports will fail in Bun and Deno environments
39+
- Goes against the cross-runtime design principle
40+
- The `any` type for runtime-specific servers is actually the correct pattern here
41+
42+
**Current state:**
43+
44+
- Build fails without `@ts-ignore` for Deno globals
45+
- The added type packages aren't fully utilized
46+
- Node-specific type imports break cross-runtime compatibility
47+
48+
### 2. **Type Safety Could Be Stronger**
49+
50+
While the base class reduces duplication, we're still using `any` for runtime-specific server types (e.g., `Bun.Server`). With the type packages added, we could potentially use the proper types:
51+
52+
```typescript
53+
private server?: Bun.Server; // Instead of any
54+
```
55+
56+
### 3. **Minor Implementation Details**
57+
58+
- The `generateCallbackHTML` function refactor is cleaner but changes the logic flow slightly (early return vs. nested if)
59+
- The Node.js implementation now uses `req.headers.host` which is good, but might need validation for security
60+
61+
## 📊 Test Results
62+
63+
✅ All tests pass successfully (9/9 tests, 22 assertions)
64+
✅ Examples run without issues
65+
✅ Server functionality remains intact
66+
67+
## 🎯 Suggestions for Improvement
68+
69+
1. **Consider conditional type imports** instead of adding all type packages:
70+
71+
```typescript
72+
/// <reference types="bun-types" />
73+
```
74+
75+
Only in files where needed, keeping the package lighter.
76+
77+
2. **Type the server properties properly** if keeping the type packages:
78+
79+
```typescript
80+
private server?: Bun.Server; // For BunCallbackServer
81+
private server?: HttpServer; // For NodeCallbackServer
82+
```
83+
84+
3. **Document the breaking changes** if any - the refactoring might affect error messages or timing slightly.
85+
86+
## 🚀 Overall Assessment
87+
88+
This is a **high-quality PR** that brings meaningful improvements to the codebase! The architectural changes with the base class pattern and modernized Promise handling are exactly what this project needed. While there are some minor issues with the TypeScript types that need resolution, the core improvements are solid and valuable.
89+
90+
The code is cleaner, more maintainable, and follows modern JavaScript/TypeScript best practices. Your attention to detail in areas like resource cleanup and error handling shows careful consideration of edge cases.
91+
92+
## 📝 Recommendation
93+
94+
I recommend **accepting this PR with modifications**:
95+
96+
1. **Remove the Node.js type imports** (`import type { Server as HttpServer } from "node:http"`) - these break cross-runtime compatibility
97+
2. Either remove the unused type packages or keep `any` types for runtime-specific code
98+
3. Ensure TypeScript compilation works without errors
99+
4. Consider adding a comment about the Map-based listener approach for future maintainers
100+
101+
Thank you again for this excellent contribution! Your efforts to modernize the codebase while maintaining backward compatibility are truly appreciated. The improvements you've made will benefit all users of this library. 🙏
102+
103+
Keep up the great work, and I look forward to any future contributions you might have!
104+
105+
---
106+
107+
_Review by @koistya_

src/index.ts

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -15,15 +15,13 @@ export type { CallbackResult, CallbackServer, ServerOptions } from "./server";
1515
export { OAuthError } from "./errors";
1616
export type { GetAuthCodeOptions } from "./types";
1717

18-
// MCP auth providers
19-
export { browserAuth } from "./auth/browser-auth";
20-
21-
// Storage implementations
18+
// Storage implementations (backward compatibility)
2219
export { inMemoryStore } from "./storage/memory";
2320
export { fileStore } from "./storage/file";
2421

25-
// MCP types
26-
export type { BrowserAuthOptions, Tokens, TokenStore } from "./mcp-types";
22+
// MCP namespace export
23+
import * as mcp from "./mcp";
24+
export { mcp };
2725

2826
/**
2927
* Captures OAuth authorization code via localhost callback.

src/mcp.ts

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
/* SPDX-FileCopyrightText: 2025-present Kriasoft */
2+
/* SPDX-License-Identifier: MIT */
3+
4+
/**
5+
* MCP SDK-specific OAuth providers and utilities.
6+
* For Model Context Protocol integration.
7+
*
8+
* @module oauth-callback/mcp
9+
*/
10+
11+
export { browserAuth } from "./auth/browser-auth";
12+
13+
export { inMemoryStore } from "./storage/memory";
14+
export { fileStore } from "./storage/file";
15+
16+
export type {
17+
BrowserAuthOptions,
18+
Tokens,
19+
TokenStore,
20+
ClientInfo,
21+
OAuthSession,
22+
OAuthStore,
23+
} from "./mcp-types";

0 commit comments

Comments
 (0)