Fix TypeScript setEndpoint validation for undefined values #1261
Conversation
WalkthroughThis pull request adds early input validation to endpoint setter methods in four SDK template files (Deno, Node, React Native, and Web). Each change verifies that the provided endpoint value is a non-empty string and throws a dedicated exception if invalid, before performing the existing URL-scheme checks. For React Native and Web the same non-empty string guard was also added to the realtime endpoint setter. No public signatures or return types were changed; existing URL-prefix validation logic remains in place. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Points to review:
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
🔇 Additional comments (2)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
templates/node/src/client.ts.twig (1)
98-105: New runtime guard fixes the TypeError; double‑check semantics for empty strings and non‑primitive stringsThe added check:
if (!endpoint || typeof endpoint !== 'string') { throw new {{spec.title | caseUcfirst}}Exception('Endpoint must be a valid string'); }correctly prevents
.startsWithbeing called onundefined/null/non‑string values and gives a clear error message, which matches the PR goal.Two small behavioral nuances to be aware of:
''(empty string) is now treated as “not a valid string” and throws the first error, whereas previously it would fall through and be rejected by the URL‑prefix check with the"Invalid endpoint URL: "message. If you care about preserving that distinction, you could instead guard only on type and let empty/whitespace strings be rejected by the URL check.new String('https://...')(boxed string) will now be rejected becausetypeof endpoint === 'object', even though it has.startsWith. This is probably fine (and arguably desirable), but it’s a subtle behavior change.If you want to keep the current bugfix but narrow the guard to just non‑strings, something like the following would preserve more of the old behavior while still fixing
undefined/null/non‑string inputs:- setEndpoint(endpoint: string): this { - if (!endpoint || typeof endpoint !== 'string') { - throw new {{spec.title | caseUcfirst}}Exception('Endpoint must be a valid string'); - } + setEndpoint(endpoint: string): this { + if (typeof endpoint !== 'string') { + throw new {{spec.title | caseUcfirst}}Exception('Endpoint must be a valid string'); + } @@ - if (!endpoint.startsWith('http://') && !endpoint.startsWith('https://')) { + if (!endpoint.startsWith('http://') && !endpoint.startsWith('https://')) { throw new {{spec.title | caseUcfirst}}Exception('Invalid endpoint URL: ' + endpoint); }Non‑blocking, just something to confirm against how you expect existing callers (especially any that might pass
'') to behave.templates/web/src/client.ts.twig (1)
336-343: HTTP endpoint validation looks good; consider mirroring it for realtime and revisiting empty‑string behaviorThe new guard in
setEndpoint:if (!endpoint || typeof endpoint !== 'string') { throw new {{spec.title | caseUcfirst}}Exception('Endpoint must be a valid string'); }nicely prevents
.startsWithonundefined/null/non‑string values and gives the clearer error message required by the PR.As with the Node template, this also changes behavior for
'':
''now throws “Endpoint must be a valid string” instead of “Invalid endpoint URL: ”. If you prefer to treat empty strings as “a string but not a valid URL,” you could guard only ontypeof endpoint !== 'string'and let the URL check handle empties.Additionally, for consistency and to avoid the same kind of TypeError on the realtime endpoint, you might want to add a similar guard to
setEndpointRealtime:setEndpointRealtime(endpointRealtime: string): this { - if (!endpointRealtime.startsWith('ws://') && !endpointRealtime.startsWith('wss://')) { + if (typeof endpointRealtime !== 'string' || !endpointRealtime) { + throw new {{spec.title | caseUcfirst}}Exception('Endpoint must be a valid string'); + } + + if (!endpointRealtime.startsWith('ws://') && !endpointRealtime.startsWith('wss://')) { throw new {{spec.title | caseUcfirst}}Exception('Invalid realtime endpoint URL: ' + endpointRealtime); }That would give users symmetric, SDK‑level validation for both HTTP and realtime endpoints instead of an untyped
.startsWithfailure. Non‑blocking, since this is pre‑existing behavior rather than a regression.templates/deno/src/client.ts.twig (1)
48-55: Deno guard is consistent and fixes the bug; same edge‑case considerations as other templatesThe new Deno implementation:
setEndpoint(endpoint: string): this { if (!endpoint || typeof endpoint !== 'string') { throw new {{spec.title | caseUcfirst}}Exception('Endpoint must be a valid string'); } if (!endpoint.startsWith('http://') && !endpoint.startsWith('https://')) { throw new {{spec.title | caseUcfirst}}Exception('Invalid endpoint URL: ' + endpoint); } ... }correctly guards against
undefined/null/non‑string values and replaces the previous raw.startsWithfailure with a clear SDK exception, which aligns with the PR’s intent.Same optional nuances as in the Node/Web templates:
''now yields “Endpoint must be a valid string” instead of “Invalid endpoint URL: ”.- Boxed strings (
new String('https://...')) will be rejected as non‑string even though they have.startsWith.If you’d like behavior closer to the previous implementation while still fixing the TypeError, you could narrow the guard to the type check only:
- setEndpoint(endpoint: string): this { - if (!endpoint || typeof endpoint !== 'string') { - throw new {{spec.title | caseUcfirst}}Exception('Endpoint must be a valid string'); - } + setEndpoint(endpoint: string): this { + if (typeof endpoint !== 'string') { + throw new {{spec.title | caseUcfirst}}Exception('Endpoint must be a valid string'); + }Not a blocker—just something to align across all templates if desired.
templates/react-native/src/client.ts.twig (1)
131-138: React Native HTTP endpoint guard is good; consider aligning realtime endpoint validationThe new code:
setEndpoint(endpoint: string): this { if (!endpoint || typeof endpoint !== 'string') { throw new {{spec.title | caseUcfirst}}Exception('Endpoint must be a valid string'); } if (!endpoint.startsWith('http://') && !endpoint.startsWith('https://')) { throw new {{spec.title | caseUcfirst}}Exception('Invalid endpoint URL: ' + endpoint); } ... }fixes the original
.startsWithonundefinedproblem and matches the behavior/messages of the other templates. The same optional considerations apply as elsewhere (empty string vs. URL error, boxed strings).For consistency and to avoid the same category of issue on the realtime endpoint, you might want to add a similar guard to
setEndpointRealtime:setEndpointRealtime(endpointRealtime: string): this { - if (!endpointRealtime.startsWith('ws://') && !endpointRealtime.startsWith('wss://')) { + if (typeof endpointRealtime !== 'string' || !endpointRealtime) { + throw new {{spec.title | caseUcfirst}}Exception('Endpoint must be a valid string'); + } + + if (!endpointRealtime.startsWith('ws://') && !endpointRealtime.startsWith('wss://')) { throw new {{spec.title | caseUcfirst}}Exception('Invalid realtime endpoint URL: ' + endpointRealtime); }That would give users the same clear, SDK‑level error for bad realtime endpoints instead of a raw runtime error. Non‑blocking; can be folded into this PR or a follow‑up.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
templates/deno/src/client.ts.twig(1 hunks)templates/node/src/client.ts.twig(1 hunks)templates/react-native/src/client.ts.twig(1 hunks)templates/web/src/client.ts.twig(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: build (8.3, WebNode)
- GitHub Check: build (8.3, WebChromium)
- GitHub Check: build (8.3, KotlinJava17)
- GitHub Check: build (8.3, Python39)
- GitHub Check: build (8.3, AppleSwift56)
- GitHub Check: build (8.3, Ruby27)
- GitHub Check: build (8.3, Python311)
- GitHub Check: build (8.3, Node16)
- GitHub Check: build (8.3, KotlinJava8)
- GitHub Check: build (8.3, FlutterStable)
- GitHub Check: build (8.3, DartStable)
- GitHub Check: build (8.3, KotlinJava11)
- GitHub Check: build (8.3, Android5Java17)
- GitHub Check: build (8.3, Android14Java17)
- GitHub Check: kotlin (server)
- GitHub Check: react-native (client)
- GitHub Check: android (client)
- GitHub Check: apple (client)
- GitHub Check: node (server)
- GitHub Check: swift (server)
Previously, calling setEndpoint() with undefined would throw a confusing TypeError when trying to call .startsWith() on undefined. This adds proper validation to check if the endpoint is null, undefined, or not a string before checking its format, providing a clear error message. Changes: - Add endpoint validation before .startsWith() check - Throw descriptive error: "Endpoint must be a valid string" - Applied to Node, Web, Deno, and React Native TypeScript clients Fixes the weird error when endpoint is undefined.
ChiragAgg5k
force-pushed
the
fix-typescript-setendpoint-validation
branch
from
36b9839 to
bfa2fb0
Compare
Summary
Fixes the weird error thrown when
setEndpoint()is called withundefinedin TypeScript client SDKs.Problem
Previously, calling
setEndpoint(undefined)would throw a confusingTypeErrorbecause the code tried to call.startsWith()onundefined:Solution
Added proper validation to check if the endpoint parameter is:
nullundefinedBefore attempting to validate the URL format. Now users get a clear, descriptive error message:
Changes
.startsWith()checktemplates/node/src/client.ts.twig)templates/web/src/client.ts.twig)templates/deno/src/client.ts.twig)templates/react-native/src/client.ts.twig)Test plan
undefined:client.setEndpoint(undefined)→ Should throw "Endpoint must be a valid string"null:client.setEndpoint(null)→ Should throw "Endpoint must be a valid string"client.setEndpoint('invalid')→ Should throw "Invalid endpoint URL: invalid"client.setEndpoint('https://cloud.appwrite.io/v1')→ Should work correctlySummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.