refactor: enhance TypeScript definitions#185
Conversation
- Introduced a new `tsconfig.json` for TypeScript configuration. - Added `typecheck` script to `package.json` for type checking. - Updated `package.json` to include TypeScript and @types/node as dev dependencies. - Modified GitHub Actions workflow to include type checking step. This was tested by intentionally adding a typo to the definition file and running tsc until the output was what I expected
- Modified the `Callback` type to accept `Error | null` and optional `payload`. - Updated method signatures in various routes to use more specific argument types, replacing `any` with `DetailRoute` or `AthleteRouteArgs` where applicable. - Enhanced return types for methods in `RoutesRoutes` and `OAuthRoutes` for better type clarity. - Adjusted the `segments` property in `AthleteRouteResponse` to use `unknown[]` instead of `any[]` for stricter type enforcement.
- Replaced the inline athlete type in `ClubActivity` and `DetailedActivityResponse` with a new `MetaAthlete` interface for improved type consistency and clarity.
- Modified `UploadRouteArgs` to allow `file` as either a string or Buffer, and made `name` and `external_id` optional. - Renamed `athlete_id` to `id` in `AthleteRouteArgs` for consistency. - `AthleteRoutes` and `OAuthRoutes` no longer accept the `done` callback.
…ting - Added JSDoc comments for better type checking and documentation in `axiosUtility.js`. - Improved error handling with detailed parameter descriptions in `StatusCodeError` and `RequestError`. - Updated `httpRequest` function to include comprehensive parameter types and return types. - Refactored `rateLimiting.js` to include TypeScript type definitions for rate limit management, enhancing clarity and maintainability. - Added methods for parsing and updating rate limits with appropriate type annotations.
- Added comprehensive JSDoc comments and TypeScript type definitions across various files, including `axiosUtility.js`, `index.d.ts` - Updated method signatures to remove callback parameters, ensuring all API methods return Promises for consistency. - Updated README to reflect changes in API usage, emphasizing the Promise-based approach. - Renames listKudos to listKudoers to more accurately describe its return type
📝 WalkthroughWalkthroughThis PR converts the library from callback-based to Promise-based APIs across all route handlers and HTTP utilities. It adds TypeScript configuration, updates error handling to use Axios-based error types, introduces a new authenticated client method, and expands the type definitions. The README is updated to reflect Promise-only usage. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
README.md (1)
313-313:⚠️ Potential issue | 🟡 MinorThe error handling example uses an internal path that npm consumers cannot access.
require('./axiosUtility')refers to a library-internal file. When installing via npm, consumers cannot import from internal paths. The README should either:
- Export
StatusCodeErrorandRequestErrorfrom the main entry point (index.js) and update the example to use the public API, or- Clarify that consumers must import directly from the internal file, or
- Show how consumers should catch and identify these errors using the thrown error's
nameproperty orinstanceofchecks without relying on importing the classes.lib/httpClient.js (1)
373-402:⚠️ Potential issue | 🟠 MajorMissing body-extraction path for
FullResponsewith abodyproperty.The request handler contract allows returning
FullResponse | string | object. When a request handler returns aFullResponsewith a truthybodyproperty, the current code skips lines 382–386 and falls through to line 402, returning the entire wrapper object (including headers) instead of extracting and parsing the body. Route modules expect the resolved value to be the API payload, not the{ headers, body }wrapper.Suggested fix: extract and parse `body` from FullResponse
// if the client auto-parsed JSON, response is already an object: if (typeof response === 'object' && response !== null && !(/** `@type` {FullResponse} */ (response).body)) { return response } + // If the response is a FullResponse with a body, extract and parse the body + if (typeof response === 'object' && response !== null && /** `@type` {FullResponse} */ (response).body) { + const body = /** `@type` {FullResponse} */ (response).body + if (typeof body === 'string') { + if (options.responseType === 'text') { + return body + } + if (options.responseType === 'json' || options.responseType === 'formdata') { + return /** `@type` {object} */ (JSONbig.parse(body)) + } + return body + } + return body + } + if (typeof response === 'string') {index.d.ts (3)
32-35:⚠️ Potential issue | 🟠 Major
CreatePushSubscriptionRouteArgsrequiresclient_idandclient_secret, but the implementation injects them from the authenticator.In
lib/pushSubscriptions.js(lines 67-70),client_idandclient_secretare populated fromauthenticator.getClientId()/authenticator.getClientSecret(), not from the caller's args. ExtendingApplicationBaseArgsforces callers to provide credentials they don't need to pass.Proposed fix
-export interface CreatePushSubscriptionRouteArgs extends ApplicationBaseArgs { +export interface CreatePushSubscriptionRouteArgs { callback_url: string; verify_token: string; }
19-26:⚠️ Potential issue | 🟡 Minor
Datetype used for fields that arrive as JSON strings.
ListPushSubscriptionResponse.created_at/updated_at(lines 24-25),SummaryAthlete.created_at/updated_at(lines 437-438), andDetailedActivity.start_date/start_date_local(lines 673-674) are typed asDate, but the Strava API returns ISO 8601 strings andJSON.parsedoes not auto-hydrate them intoDateobjects. These should bestringto match runtime behavior.Also applies to: 437-438, 673-674
907-926:⚠️ Potential issue | 🟠 Major
Strava.client()return type is incorrect — should not bevoid.The JSDoc comment states "Call with
new strava.client(token, request)", and the implementation (lines 38-66 of index.js) confirms this is a constructor function that mutatesthiswith properties likeaccess_token,athlete,athletes,activities, etc. When invoked withnew, the constructor implicitly returns the configured instance, notvoid. The type signature should reflect this by using TypeScript's constructor signature syntax (e.g.,new (token: string, request?: RequestHandler): <StravaInstance>) rather than declaring a method that returnsvoid.
🤖 Fix all issues with AI agents
In `@index.d.ts`:
- Around line 162-180: The SummarySegment interface currently marks
athlete_pr_effort and athlete_segment_stats as required but they can be absent
for summary segments; update the SummarySegment definition to make
athlete_pr_effort and athlete_segment_stats optional (i.e., add ? to those
property names) so their types SummaryPRSegmentEffort and SummarySegmentEffort
reflect that they may be undefined when the authenticated athlete has no
efforts.
- Around line 354-382: The current StreamSet and StreamsRoutes types are
incorrect: StreamSet models all streams as required and StreamsRoutes methods
return Promise<StreamSet[]>, but the API returns an array of individual stream
objects. Replace the StreamSet usage by defining a union type (e.g., Stream =
TimeStream | DistanceStream | LatLngStream | AltitudeStream |
SmoothVelocityStream | HeartrateStream | CadenceStream | PowerStream |
TemperatureStream | MovingStream | SmoothGradeStream) and update
StreamsRoutes.activity/route/effort/segment to return Promise<Stream[]>;
alternatively, if you intend to keep StreamSet, make all properties optional on
StreamSet and update the return types accordingly (but prefer the union
approach).
In `@lib/athlete.js`:
- Around line 56-59: The JSDoc cast is applied to the already-awaited value in
athlete.prototype.get (and similarly at the other return sites around lines 105
and 119); update each return to either cast the promise before awaiting (e.g.,
apply /** `@type` {Promise<SummaryAthlete>} */ to this.client.getEndpoint(...) and
then await it) or change the JSDoc to the unwrapped type and cast the resolved
value (e.g., /** `@type` {SummaryAthlete} */ after await); apply the same
consistent fix to the other methods that currently do the post-await Promise<T>
cast.
In `@lib/clubs.js`:
- Around line 23-33: The synchronous throw in clubs.prototype.get (and the
helper _listHelper used by listMembers, listActivities, listAdmins) causes
uncaught exceptions when callers expect a Promise; change these to consistently
return rejected Promises instead of throwing synchronously by either marking
clubs.prototype.get and _listHelper as async (so thrown errors become rejected
promises) or replacing the throw statements with return Promise.reject(new
Error(...)); ensure every code path in get, _listHelper, listMembers,
listActivities, and listAdmins returns a Promise so callers can safely use
.catch().
In `@lib/pushSubscriptions.js`:
- Around line 50-53: pushSubscriptions.prototype.create (and similarly
pushSubscriptions.prototype.delete) currently throws synchronously when required
args are missing; change this to return Promise.reject(new Error(...)) so
validation failures are delivered as rejected Promises consistent with
segments.js and callers using .catch/await will receive a promise rejection.
Locate the argument checks in pushSubscriptions.prototype.create and
pushSubscriptions.prototype.delete and replace the synchronous throw statements
with returns of Promise.reject(new Error('required args missing: callback_url
and verify_token are required')) (or the appropriate error message used in
segments.js) so behavior matches the rest of the module.
In `@lib/segmentEfforts.js`:
- Around line 23-25: Update the stale inline comment above the args.id
validation in the module (the comment currently reads "// require segment id")
to accurately reflect the check and error message by changing it to something
like "// require segment effort id" so it matches the thrown Error in the block
that validates args.id.
In `@lib/uploads.js`:
- Around line 67-73: The current falsy check in the loop over _allowedFormProps
uses if (argsRecord[key]) which drops valid numeric 0 values (notably for
trainer and commute); change the condition to explicitly test for presence such
as if (argsRecord[key] != null) or if (argsRecord[key] !== undefined) so that 0
is preserved when copying into args.formData, keeping the rest of the loop and
variables (argsRecord, _allowedFormProps, args.formData) unchanged.
In `@README.md`:
- Line 227: Fix the typo in the README where the description for
strava.oauth.getToken(code) reads "Used to token exchange" — update the phrase
to "Used for token exchange" so the entry for the symbol
strava.oauth.getToken(code) reads correctly.
- Line 284: The phrase "comma separated" in the README usage description for
strava.segments.explore(args) should be hyphenated as "comma-separated"; update
the README entry that documents strava.segments.explore(args) so the compound
adjective reads `"Expects arg `bounds` as a comma-separated string..."` (modify
the text around the function reference `strava.segments.explore(args)`
accordingly).
🧹 Nitpick comments (12)
tsconfig.json (2)
13-17:outDirandsourceMapare redundant whennoEmitis true.Since
noEmit: trueprevents any output,outDir: "dist"andsourceMap: truehave no effect. Consider removing them to reduce config noise.Proposed cleanup
// Emit "declaration": false, "declarationMap": false, "noEmit": true, - "outDir": "dist", // JavaScript Support "allowJs": true, "checkJs": true, // Language and Environment "target": "es2022", - "sourceMap": true,
27-28:skipLibCheck: falsewill type-check all.d.tsfiles innode_modules.This is thorough but can noticeably slow down type-checking, especially as dependencies grow. If typecheck times become a concern, flipping this to
trueis the standard recommendation.lib/athletes.js (1)
24-41:_listHelperis now constrained to only'stats'— consider simplifying.The
listTypeparameter is typed as the literal'stats', making the indirection through_listHelperunnecessary. If no other sub-resources will use this helper, inlining the logic intostats()would be simpler. If future expansion is planned (e.g.,'koms'), consider broadening the type.Also,
getPaginationQSis called for the stats endpoint, which typically returns a single object rather than a paginated list — this is pre-existing behavior but worth noting.lib/authenticator.js (1)
89-147: RepeatedfetchConfig()calls on every getter invocation when value is undefined.Each getter (e.g.,
getToken,getClientId) callsfetchConfig()every time the value is falsy, which re-reads the config file and environment variables. If the credential is legitimately absent, this I/O cost recurs on every call. A simpleconfigLoadedflag would avoid redundant reads.Sketch of a configLoaded guard
+var configLoaded = false + var fetchConfig = function (config) { if (config) { if (config.access_token) token = config.access_token if (config.client_id) clientId = config.client_id if (config.client_secret) clientSecret = config.client_secret if (config.redirect_uri) redirectUri = config.redirect_uri } else { readConfigFile() readEnvironmentVars() } + configLoaded = true }Then in each getter, replace
if (!token)withif (!configLoaded):getToken: function () { - if (!token) { + if (!configLoaded) { fetchConfig() } // ...And in
purge:purge: function () { token = undefined clientId = undefined clientSecret = undefined redirectUri = undefined + configLoaded = false }lib/rateLimiting.js (2)
157-182:updateRateLimitsclears all state when headers are missing — verify this is intentional.When
parseRateLimitsreturnsnull(e.g. a response lacking one of the four required headers),updateRateLimitscallsthis.clear(), which resets all counters to zero. This means a single response without complete rate-limit headers erases the previously known limit/usage state.If the intent is to be conservative (assume unknown = safe), this is fine. But if a consumer is tracking cumulative rate state and an intermediary error response omits headers, silently clearing could mask that the rate limit was already nearly exceeded.
Consider returning
nullwithout clearing state, or at least documenting the clearing behavior:Possible alternative: preserve state on missing headers
} else { - this.clear() + // Headers missing/invalid — preserve existing state }
46-46: Minor inconsistency:rlalias vsthisacross methods.
exceeded/fractionReached/readExceeded/readFractionReachedread state via the module-levelrlalias, whileupdateRateLimitsandclearmutate state viathis. This works becauserl === RateLimit, but mixing the two patterns can confuse future maintainers. Consider standardizing on one approach.Also applies to: 53-63, 158-159
lib/httpClient.js (1)
243-271:postUpload: solid upload implementation with one typedef nit.The validation, stream creation, and header merging look correct. Minor note: the typedef for
fileisstring|Buffer, butfs.createReadStreamtreats a Buffer as a file path (not file contents). Consider clarifying in the JSDoc thatfileis a file path.Suggested JSDoc clarification
- * `@property` {string|Buffer} [file] + * `@property` {string} file - File path to upload (e.g., 'data/your_file.gpx')index.js (1)
29-66: Constructor lacksnew-call guard
strava.clientis intended to be called withnew(as the JSDoc states), but nothing prevents it from being called as a plain function. Withoutnew,thisrefers to the caller's context (orundefinedin strict mode), silently corrupting state instead of throwing.Consider adding a guard or converting to a class:
🛡️ Proposed guard
strava.client = function (token, request = httpRequest) { + if (!(this instanceof strava.client)) { + return new strava.client(token, request) + } this.access_token = tokenlib/oauth.js (1)
75-89:json: trueis a no-op with the axios-basedhttpRequestThe
json: trueproperty (Lines 79, 101, 125) is a leftover from therequest-promiseAPI. The axios wrapper inaxiosUtility.jsalready defaultsresponseTypeto'json'and doesn't read ajsonproperty. It's also absent from theHttpRequestOptionstypedef. Consider removing it to avoid confusion.♻️ Cleanup in all three methods
// In getToken (line 79): method: 'POST', url: 'https://www.strava.com/oauth/token', - json: true, qs: { // In deauthorize (line 101): url: 'https://www.strava.com/oauth/deauthorize', method: 'POST', - json: true, simple: false, // In refreshToken (line 125): url: 'https://www.strava.com/oauth/token', method: 'POST', - json: true, simple: true,axiosUtility.js (1)
94-129:options.maxRedirects || 5treats explicit0as falsyLine 108: if a caller explicitly passes
maxRedirects: 0, the||operator falls through to5. Consider using nullish coalescing (??) instead if you want to allow0.This is pre-existing behavior, so feel free to defer.
♻️ Suggested fix
- maxRedirects: options.maxRedirects || 5, + maxRedirects: options.maxRedirects ?? 5,lib/uploads.js (1)
99-120: Unbounded recursive polling could exhaust the stack on a stuck upload
_checkrecursively calls itself every second until the upload is done. If the Strava API never transitions out of the "processing" state (e.g., a hung upload), this will recurse indefinitely. Consider adding a max-retry limit or converting to an iterative loop.This appears to be pre-existing behavior, so feel free to defer.
lib/pushSubscriptions.js (1)
86-98:list()silently succeeds with empty credentials if authenticator is not configured.Lines 90-91 fall back to
''whengetClientSecret()/getClientId()return null. This means a misconfigured client will send an API request with empty credentials and get an opaque Strava error instead of a clear local error. This is a pre-existing pattern applied consistently across all three methods, so it's not a blocker, but worth noting for future improvement.
| export interface SummarySegment { | ||
| id: number; | ||
| name: string; | ||
| activity_type: string; | ||
| distance: number; | ||
| average_grade?: number; | ||
| maximum_grade?: number; | ||
| elevation_high?: number; | ||
| elevation_low?: number; | ||
| start_latlng?: LatLng; | ||
| end_latlng?: LatLng; | ||
| climb_category?: number; | ||
| city?: string; | ||
| state?: string; | ||
| country?: string; | ||
| private?: boolean; | ||
| athlete_pr_effort: SummaryPRSegmentEffort; | ||
| athlete_segment_stats: SummarySegmentEffort; | ||
| } |
There was a problem hiding this comment.
SummarySegment.athlete_pr_effort and athlete_segment_stats should be optional.
These fields (lines 178-179) are marked as required, but a summary segment returned from listStarred or as a nested object may not always include these athlete-specific stats (they depend on the authenticated athlete having efforts on the segment).
🤖 Prompt for AI Agents
In `@index.d.ts` around lines 162 - 180, The SummarySegment interface currently
marks athlete_pr_effort and athlete_segment_stats as required but they can be
absent for summary segments; update the SummarySegment definition to make
athlete_pr_effort and athlete_segment_stats optional (i.e., add ? to those
property names) so their types SummaryPRSegmentEffort and SummarySegmentEffort
reflect that they may be undefined when the authenticated athlete has no
efforts.
| export interface StreamSet { | ||
| time: TimeStream; | ||
| distance: DistanceStream; | ||
| latlng: LatLngStream; | ||
| altitude: AltitudeStream; | ||
| velocity_smooth: SmoothVelocityStream; | ||
| heartrate: HeartrateStream; | ||
| cadence: CadenceStream; | ||
| watts: PowerStream; | ||
| temp: TemperatureStream; | ||
| moving: MovingStream; | ||
| grade_smooth: SmoothGradeStream; | ||
| } | ||
|
|
||
| /** Args for stream endpoints (id required; optional keys, key_by_type, etc.). */ | ||
| export interface StreamsArgs extends DetailRoute { | ||
| keys?: string[]; | ||
| key_by_type?: boolean; | ||
| original_size?: boolean; | ||
| resolution?: string; | ||
| series_type?: string; | ||
| } | ||
|
|
||
| export interface StreamsRoutes { | ||
| activity(args: any, done?: Callback): Promise<any>; | ||
| effort(args: any, done?: Callback): Promise<any>; | ||
| segment(args: any, done?: Callback): Promise<any>; | ||
| route(args: any, done?: Callback): Promise<any>; | ||
| activity(args: StreamsArgs): Promise<StreamSet[]>; | ||
| route(args: StreamsArgs): Promise<StreamSet[]>; | ||
| effort(args: StreamsArgs): Promise<StreamSet[]>; | ||
| segment(args: StreamsArgs): Promise<StreamSet[]>; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Strava API streams endpoint response format documentation
💡 Result:
Strava’s Streams endpoints (e.g. GET /api/v3/activities/{id}/streams, .../segments/{id}/streams, .../segment_efforts/{id}/streams) return a StreamSet—in the reference docs this is shown as a JSON array of stream objects, one per requested stream type. Each stream object has the shape: type, data, series_type, original_size, resolution. [1]
Per-stream object fields (response format):
type(string): the stream type (e.g.time,latlng,distance,altitude,velocity_smooth,heartrate,cadence,watts,temp,moving,grade_smooth). [1]data(array): the samples for that stream (e.g.,latlngis an array of[lat, lng]pairs;distanceis floats;timeis ints). [1]series_type(string enum): base series used if downsampled:distanceortime. [1]original_size(integer): number of points in the original stream. [1]resolution(string enum): sampling detail level:low,medium,high. [1]
Example (from Strava docs):
[
{
"type": "distance",
"data": [2.9, 5.8, 8.5],
"series_type": "distance",
"original_size": 12,
"resolution": "high"
}
][1]
If you want a machine-readable schema, stravalib mirrors the same core fields in its Stream model (helpful for typing/validation). [2]
Sources: Strava API Reference (Streams + BaseStream model + sample responses) [1]; stravalib Stream JSON schema [2].
[1] (developers.strava.com)
[2] (stravalib.readthedocs.io)
Citations:
- 1: https://developers.strava.com/docs/reference/
- 2: https://stravalib.readthedocs.io/en/v2.3/reference/api/stravalib.model.Stream.html?utm_source=openai
🏁 Script executed:
# Check the streams implementation to understand how the API response is handled
fd -t f "streams" --type f | head -20Repository: node-strava/node-strava-v3
Length of output: 100
🏁 Script executed:
# Find and examine streams.js to understand the response handling
find . -name "streams.js" -o -name "streams.ts" | head -10Repository: node-strava/node-strava-v3
Length of output: 104
🏁 Script executed:
cat -n ./lib/streams.jsRepository: node-strava/node-strava-v3
Length of output: 3483
🏁 Script executed:
# Also check the type definitions around the streams
sed -n '354,382p' index.d.ts | cat -nRepository: node-strava/node-strava-v3
Length of output: 1104
🏁 Script executed:
# Get more context around the stream types in the index.d.ts
sed -n '266,382p' index.d.ts | cat -nRepository: node-strava/node-strava-v3
Length of output: 3665
🏁 Script executed:
# Let's check how the response is actually used in the test file
cat -n ./test/streams.js | head -100Repository: node-strava/node-strava-v3
Length of output: 3795
🏁 Script executed:
# Let's continue reading the test to see more patterns
sed -n '94,150p' ./test/streams.js | cat -nRepository: node-strava/node-strava-v3
Length of output: 2200
Fix return type: Strava API streams endpoint returns an array of individual stream objects, not StreamSet[].
The Strava API streams endpoints (e.g., GET /api/v3/activities/{id}/streams) return an array of individual stream objects, each with type, data, series_type, original_size, and resolution fields. For example:
[
{"type": "distance", "data": [2.9, 5.8, 8.5], "series_type": "distance", "original_size": 12, "resolution": "high"},
{"type": "latlng", "data": [[37.83, -122.48], ...], ...}
]However, the type definitions claim Promise<StreamSet[]>, where StreamSet is defined as an object containing all 11 stream properties as required. This is incorrect:
StreamSetmodel is wrong: It defines all properties as required (e.g.,time: TimeStream,distance: DistanceStream), but the API only returns the requested stream types.- Return type is wrong:
Promise<StreamSet[]>means "an array of objects, each containing all 11 streams", which doesn't match the actual API response format.
The correct fix is to define a union type for individual streams and use that as the return type:
Recommended fix
export type Stream = TimeStream | DistanceStream | LatLngStream | AltitudeStream
| SmoothVelocityStream | HeartrateStream | CadenceStream | PowerStream
| TemperatureStream | MovingStream | SmoothGradeStream;
export interface StreamsRoutes {
activity(args: StreamsArgs): Promise<Stream[]>;
route(args: StreamsArgs): Promise<Stream[]>;
effort(args: StreamsArgs): Promise<Stream[]>;
segment(args: StreamsArgs): Promise<Stream[]>;
}Alternatively, if StreamSet is intended to represent aggregated streams (though the API doesn't work this way), all its properties should be optional to reflect that only requested streams are returned.
🤖 Prompt for AI Agents
In `@index.d.ts` around lines 354 - 382, The current StreamSet and StreamsRoutes
types are incorrect: StreamSet models all streams as required and StreamsRoutes
methods return Promise<StreamSet[]>, but the API returns an array of individual
stream objects. Replace the StreamSet usage by defining a union type (e.g.,
Stream = TimeStream | DistanceStream | LatLngStream | AltitudeStream |
SmoothVelocityStream | HeartrateStream | CadenceStream | PowerStream |
TemperatureStream | MovingStream | SmoothGradeStream) and update
StreamsRoutes.activity/route/effort/segment to return Promise<Stream[]>;
alternatively, if you intend to keep StreamSet, make all properties optional on
StreamSet and update the return types accordingly (but prefer the union
approach).
| athlete.prototype.get = async function (args) { | ||
| var endpoint = 'athlete' | ||
| return await this.client.getEndpoint(endpoint, args) | ||
| return /** @type {Promise<SummaryAthlete>} */ (await this.client.getEndpoint(endpoint, args)) | ||
| } |
There was a problem hiding this comment.
Incorrect JSDoc type cast: casting an already-awaited value as Promise<T>
On Line 58 (and similarly Lines 105, 119), the await resolves the promise first, then the /** @type {Promise<SummaryAthlete>} */ cast wraps the resolved value — this is a type mismatch. Contrast with Lines 67, 76, 85, 94 in the same file, which correctly cast the promise before await:
return await /** `@type` {Promise<DetailedActivity[]>} */ (this._listHelper('activities', args))Either cast before await, or cast the resolved value as the unwrapped type:
♻️ Option A: cast before await (consistent with lines 67/76/85/94)
- return /** `@type` {Promise<SummaryAthlete>} */ (await this.client.getEndpoint(endpoint, args))
+ return await /** `@type` {Promise<SummaryAthlete>} */ (this.client.getEndpoint(endpoint, args))Apply the same fix on Lines 105 and 119.
🤖 Prompt for AI Agents
In `@lib/athlete.js` around lines 56 - 59, The JSDoc cast is applied to the
already-awaited value in athlete.prototype.get (and similarly at the other
return sites around lines 105 and 119); update each return to either cast the
promise before awaiting (e.g., apply /** `@type` {Promise<SummaryAthlete>} */ to
this.client.getEndpoint(...) and then await it) or change the JSDoc to the
unwrapped type and cast the resolved value (e.g., /** `@type` {SummaryAthlete} */
after await); apply the same consistent fix to the other methods that currently
do the post-await Promise<T> cast.
| clubs.prototype.get = function (args) { | ||
| var endpoint = 'clubs/' | ||
|
|
||
| // require club id | ||
| if (typeof args.id === 'undefined') { | ||
| const err = { msg: 'args must include a club id' } | ||
| return done(err) | ||
| throw new Error('args must include a club id') | ||
| } | ||
|
|
||
| endpoint += args.id | ||
| return this.client.getEndpoint(endpoint, args, done) | ||
| return /** @type {Promise<DetailedClub>} */ (this.client.getEndpoint(endpoint, args)) | ||
| } |
There was a problem hiding this comment.
Synchronous throw in a non-async function breaks .catch() chaining.
get() is not async, so throw new Error(...) on line 28 raises synchronously. A caller writing strava.clubs.get({}).catch(handler) will get an uncaught exception because no Promise is ever returned. The same issue applies to _listHelper (line 74) and transitively to listMembers, listActivities, and listAdmins.
Either mark these methods async (so throws become rejected promises) or wrap the body:
Option A: mark as async (simplest)
-clubs.prototype.get = function (args) {
+clubs.prototype.get = async function (args) {-clubs.prototype._listHelper = function (listType, args) {
+clubs.prototype._listHelper = async function (listType, args) {Option B: return Promise.reject
if (typeof args.id === 'undefined') {
- throw new Error('args must include a club id')
+ return Promise.reject(new Error('args must include a club id'))
}🤖 Prompt for AI Agents
In `@lib/clubs.js` around lines 23 - 33, The synchronous throw in
clubs.prototype.get (and the helper _listHelper used by listMembers,
listActivities, listAdmins) causes uncaught exceptions when callers expect a
Promise; change these to consistently return rejected Promises instead of
throwing synchronously by either marking clubs.prototype.get and _listHelper as
async (so thrown errors become rejected promises) or replacing the throw
statements with return Promise.reject(new Error(...)); ensure every code path in
get, _listHelper, listMembers, listActivities, and listAdmins returns a Promise
so callers can safely use .catch().
| pushSubscriptions.prototype.create = function (args) { | ||
| if (typeof args.callback_url === 'undefined' || typeof args.verify_token === 'undefined') { | ||
| throw new Error('required args missing: callback_url and verify_token are required') | ||
| } |
There was a problem hiding this comment.
Inconsistent error handling: synchronous throw vs Promise.reject for validation errors.
create() and delete() (line 108) throw synchronously on missing args, whereas segments.js (e.g., lines 68, 98, 140) uses return Promise.reject(new Error(...)) for the same pattern. Since these methods return Promises, callers using only .catch() or await in a try-less context won't catch a synchronous throw — it will surface as an unhandled exception instead of a rejected Promise.
Consider aligning with the Promise.reject pattern used elsewhere in the codebase for consistency and safer caller ergonomics.
Proposed fix
pushSubscriptions.prototype.create = function (args) {
if (typeof args.callback_url === 'undefined' || typeof args.verify_token === 'undefined') {
- throw new Error('required args missing: callback_url and verify_token are required')
+ return Promise.reject(new Error('required args missing: callback_url and verify_token are required'))
} pushSubscriptions.prototype.delete = function (args) {
if (typeof args.id === 'undefined') {
- throw new Error('args must include a push subscription id')
+ return Promise.reject(new Error('args must include a push subscription id'))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pushSubscriptions.prototype.create = function (args) { | |
| if (typeof args.callback_url === 'undefined' || typeof args.verify_token === 'undefined') { | |
| throw new Error('required args missing: callback_url and verify_token are required') | |
| } | |
| pushSubscriptions.prototype.create = function (args) { | |
| if (typeof args.callback_url === 'undefined' || typeof args.verify_token === 'undefined') { | |
| return Promise.reject(new Error('required args missing: callback_url and verify_token are required')) | |
| } |
🤖 Prompt for AI Agents
In `@lib/pushSubscriptions.js` around lines 50 - 53,
pushSubscriptions.prototype.create (and similarly
pushSubscriptions.prototype.delete) currently throws synchronously when required
args are missing; change this to return Promise.reject(new Error(...)) so
validation failures are delivered as rejected Promises consistent with
segments.js and callers using .catch/await will receive a promise rejection.
Locate the argument checks in pushSubscriptions.prototype.create and
pushSubscriptions.prototype.delete and replace the synchronous throw statements
with returns of Promise.reject(new Error('required args missing: callback_url
and verify_token are required')) (or the appropriate error message used in
segments.js) so behavior matches the rest of the module.
| // require segment id | ||
| if (typeof args.id === 'undefined') { | ||
| throw new Error('args must include a segment id') | ||
| throw new Error('args must include a segment effort id') |
There was a problem hiding this comment.
Stale comment: still says "segment id" instead of "segment effort id"
The error message was correctly updated to "segment effort id", but the inline comment on Line 23 still reads // require segment id.
📝 Fix
- // require segment id
+ // require segment effort id📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // require segment id | |
| if (typeof args.id === 'undefined') { | |
| throw new Error('args must include a segment id') | |
| throw new Error('args must include a segment effort id') | |
| // require segment effort id | |
| if (typeof args.id === 'undefined') { | |
| throw new Error('args must include a segment effort id' |
🤖 Prompt for AI Agents
In `@lib/segmentEfforts.js` around lines 23 - 25, Update the stale inline comment
above the args.id validation in the module (the comment currently reads "//
require segment id") to accurately reflect the check and error message by
changing it to something like "// require segment effort id" so it matches the
thrown Error in the block that validates args.id.
| const argsRecord = /** @type {Record<string, string>} */ (/** @type {unknown} */ (args)) | ||
| for (var i = 0; i < _allowedFormProps.length; i++) { | ||
| if (args[_allowedFormProps[i]]) { args.formData[_allowedFormProps[i]] = args[_allowedFormProps[i]] } | ||
| const key = _allowedFormProps[i] | ||
| if (argsRecord[key]) { | ||
| args.formData[key] = argsRecord[key] | ||
| } | ||
| } |
There was a problem hiding this comment.
Falsy check skips valid 0 values for trainer and commute
Line 70: if (argsRecord[key]) treats 0 as falsy. The Strava API's trainer and commute fields accept 0|1, so explicitly passing 0 would be silently dropped from the form data. Use a != null (or !== undefined) check instead.
🐛 Proposed fix
- if (argsRecord[key]) {
+ if (argsRecord[key] != null) {🤖 Prompt for AI Agents
In `@lib/uploads.js` around lines 67 - 73, The current falsy check in the loop
over _allowedFormProps uses if (argsRecord[key]) which drops valid numeric 0
values (notably for trainer and commute); change the condition to explicitly
test for presence such as if (argsRecord[key] != null) or if (argsRecord[key]
!== undefined) so that 0 is preserved when copying into args.formData, keeping
the rest of the loop and variables (argsRecord, _allowedFormProps,
args.formData) unchanged.
| * `strava.oauth.getToken(code,done)` (Used to token exchange) | ||
| * `strava.oauth.refreshToken(code)` (Callback API not supported) | ||
| * `strava.oauth.deauthorize(args,done)` | ||
| * `strava.oauth.getToken(code)` (Used to token exchange) |
There was a problem hiding this comment.
Typo: "Used to token exchange" → "Used for token exchange".
-* `strava.oauth.getToken(code)` (Used to token exchange)
+* `strava.oauth.getToken(code)` (Used for token exchange)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * `strava.oauth.getToken(code)` (Used to token exchange) | |
| * `strava.oauth.getToken(code)` (Used for token exchange) |
🤖 Prompt for AI Agents
In `@README.md` at line 227, Fix the typo in the README where the description for
strava.oauth.getToken(code) reads "Used to token exchange" — update the phrase
to "Used for token exchange" so the entry for the symbol
strava.oauth.getToken(code) reads correctly.
| * `strava.segments.get(args)` | ||
| * `strava.segments.listStarred(args)` | ||
| * `strava.segments.listEfforts(args)` | ||
| * `strava.segments.explore(args)` *Expects arg `bounds` as a comma separated string, for two points describing a rectangular boundary for the search: `"southwest corner latitude, southwest corner longitude, northeast corner latitude, northeast corner longitude"`*. |
There was a problem hiding this comment.
Hyphenate "comma separated" → "comma-separated".
Per standard English grammar, compound adjectives before a noun should be hyphenated.
-* `strava.segments.explore(args)` *Expects arg `bounds` as a comma separated string, ...
+* `strava.segments.explore(args)` *Expects arg `bounds` as a comma-separated string, ...📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * `strava.segments.explore(args)` *Expects arg `bounds` as a comma separated string, for two points describing a rectangular boundary for the search: `"southwest corner latitude, southwest corner longitude, northeast corner latitude, northeast corner longitude"`*. | |
| * `strava.segments.explore(args)` *Expects arg `bounds` as a comma-separated string, for two points describing a rectangular boundary for the search: `"southwest corner latitude, southwest corner longitude, northeast corner latitude, northeast corner longitude"`*. |
🧰 Tools
🪛 LanguageTool
[grammar] ~284-~284: Use a hyphen to join words.
Context: ...(args)*Expects argbounds` as a comma separated string, for two points describ...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
In `@README.md` at line 284, The phrase "comma separated" in the README usage
description for strava.segments.explore(args) should be hyphenated as
"comma-separated"; update the README entry that documents
strava.segments.explore(args) so the compound adjective reads `"Expects arg
`bounds` as a comma-separated string..."` (modify the text around the function
reference `strava.segments.explore(args)` accordingly).
|
Thanks, go ahead and address the Code Rabbit feedback and also bump the major version in package.json. You might as well bump axios in this branch as well. (see #186) |
2 participants
axiosUtility.js,index.d.tsSummary by CodeRabbit
Release Notes
New Features
strava.client()method for creating authenticated API clientslistActivities(),listRoutes(),listClubs(),listZones(), andupdate()methodsgetRequestAccessURL()for OAuth integrationBreaking Changes
listKudos()tolistKudoers()in Activities APIImprovements