-
-
Notifications
You must be signed in to change notification settings - Fork 61
add .url() method to retrieve full route URLs #239
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
WalkthroughThis PR introduces a URL-building capability to the Treaty 2 proxy system. The implementation adds a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/treaty2.test.ts (1)
1226-1257: Good test coverage for the newurl()method.Tests cover key scenarios: base URL, path parameters, query parameters, and accessing
url()from method proxies. Consider adding edge case tests for:
- Query parameters with special characters (encoding)
- Query parameters with arrays
- Query parameters with
null/undefinedvalues (should be skipped per implementation)🔎 Optional: Additional edge case tests
it('should encode special characters in query parameters', () => { expect(client.data.url({ query: { name: 'hello world' } })).toBe('http://e.ly/data?name=hello%20world') }) it('should handle array query parameters', () => { expect(client.data.url({ query: { name: ['a', 'b'] } })).toBe('http://e.ly/data?name=a&name=b') }) it('should skip null and undefined query values', () => { expect(client.data.url({ query: { name: undefined } })).toBe('http://e.ly/data') })src/treaty2/index.ts (1)
207-245: Implementation looks correct; consider extracting shared query-building logic.The
urlproperty correctly:
- Strips HTTP method names from the path when accessed via method proxy
- Handles query parameter encoding with proper support for arrays, objects, and null/undefined skipping
However, the query string building logic (lines 217-241) is duplicated from the
applyhandler (lines 283-309). Consider extracting to a shared helper.🔎 Proposed refactor to extract query building
+const buildQueryString = (query: Record<string, any> | undefined): string => { + if (!query) return '' + + let q = '' + const append = (key: string, value: string) => { + q += + (q ? '&' : '?') + + `${encodeURIComponent(key)}=${encodeURIComponent(value)}` + } + + for (const [key, value] of Object.entries(query)) { + if (Array.isArray(value)) { + for (const v of value) append(key, v) + continue + } + + if (value === undefined || value === null) continue + + if (typeof value === 'object') { + append(key, JSON.stringify(value)) + continue + } + append(key, `${value}`) + } + + return q +} const createProxy = ( // ... ): any => new Proxy(() => {}, { get(_, param: string): any { if (param === 'url') { return (options?: { query?: Record<string, any> }) => { const methodPaths = [...paths] if (method.includes(methodPaths.at(-1) as any)) methodPaths.pop() const path = '/' + methodPaths.join('/') - - const query = options?.query - - let q = '' - if (query) { - const append = (key: string, value: string) => { - q += - (q ? '&' : '?') + - `${encodeURIComponent(key)}=${encodeURIComponent( - value - )}` - } - - for (const [key, value] of Object.entries(query)) { - if (Array.isArray(value)) { - for (const v of value) append(key, v) - continue - } - - if (value === undefined || value === null) continue - - if (typeof value === 'object') { - append(key, JSON.stringify(value)) - continue - } - append(key, `${value}`) - } - } + const q = buildQueryString(options?.query) return domain + path + q } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/treaty2/index.tssrc/treaty2/types.tstest/treaty2.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/treaty2/types.ts (3)
src/types.ts (2)
MaybeEmptyObject(64-78)Prettify(88-90)src/treaty2/ws.ts (1)
EdenWS(5-91)src/treaty/index.ts (1)
EdenWS(59-147)
🔇 Additional comments (3)
src/treaty2/types.ts (2)
97-114: LGTM! Type extension forurlproperty on routes.The type correctly:
- Adds
'url'to the mapped type keys while filtering out dynamic params (:${string})- Provides a standalone
urlsignature with looseRecord<string, any>query typing- Attaches a typed
urlbuilder to the subscribe route usingSerializeQueryParamsfor query type safety
115-187: LGTM! Type extension for HTTP routes withurlbuilder.The type correctly intersects route method signatures with a
urlbuilder that usesSerializeQueryParams<Query>for type-safe query parameters, consistent with the route's query schema.src/treaty2/index.ts (1)
681-681: LGTM!Standard type re-export.
This PR adds a
.url()method, allowing you to easily retrieve the full URL string for a route.