Skip to content

fix/use correct type declaration for props with any types - phase 1#417

Open
Bruce-Mahagwa wants to merge 2 commits intoOneBusAway:developfrom
Bruce-Mahagwa:fix/replace-any-types-with-proper-interfaces
Open

fix/use correct type declaration for props with any types - phase 1#417
Bruce-Mahagwa wants to merge 2 commits intoOneBusAway:developfrom
Bruce-Mahagwa:fix/replace-any-types-with-proper-interfaces

Conversation

@Bruce-Mahagwa
Copy link
Copy Markdown

@Bruce-Mahagwa Bruce-Mahagwa commented Mar 22, 2026

Closes #404

  • Corrected 'any' type declarations for props in the following components:
  1. src/components/containers/Accordion.svelte
  2. src/components/map/StopMarker.svelte
  3. src/components/stops/StopPane.svelte
  4. src/components/tabs/TabLink.svelte

Copy link
Copy Markdown
Member

@aaronbrethorst aaronbrethorst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review: Replace any type declarations with proper JSDoc types

Thanks for working on eliminating any types -- this is a valuable effort. I have found several issues that need to be addressed, ranging from a clear bug to architectural concerns.


1. Bug: Accordion.svelte -- items typed as {string} but used as an array

File: src/components/containers/Accordion.svelte, line 13

The type was changed from {any} to {string}, but items is unambiguously an array:

  • Initialized as $bindable([])
  • Called with items.includes(id) (line 38)
  • Spread with [...items, id] (implicit in registerItem)
  • Passed to new Set(items) (line 22)

The correct type is {string[]}.


2. Major concern: StopPane.svelte -- ~70-line inline type is the wrong approach

File: src/components/stops/StopPane.svelte, lines 22-92

The onebusaway-sdk package already ships comprehensive TypeScript definitions for every API response shape. The types you are hand-writing already exist at:

  • onebusaway-sdk/resources/arrival-and-departure.d.tsArrivalAndDepartureListResponse
  • onebusaway-sdk/resources/stop.d.tsStopRetrieveResponse
  • onebusaway-sdk/resources/shared.d.tsReferences, References.Route, References.Situation, etc.

Hand-duplicating these creates several problems:

  • Drift risk: The inline types will silently diverge from the SDK as it updates. For example, the SDK's Situation type has summary and description as nested objects ({ lang?: string, value?: string }), not plain strings as declared in this PR.
  • Incomplete coverage: The inline type omits many fields the SDK defines (e.g., arrivalEnabled, departureEnabled, stopSequence, totalStopsInTrip on arrivals; code, parent, staticRouteIds, wheelchairBoarding on stops).
  • Maintenance burden: 70 lines of inline JSDoc in a component's <script> block is difficult to read and maintain.

However, there is a structural mismatch that makes direct SDK type reuse non-trivial: the component fetches from /api/oba/arrivals-and-departures-for-stop/, which returns the entire SDK response (including the code, currentTime, text, version, data wrapper). The prop type in this PR starts at the data level, but the component actually accesses arrivalsAndDeparturesResponse.data.references.routes on line 165-166, meaning the prop contains the full response wrapper, not just data. The type as written would be wrong even structurally.

Recommendation: Rather than inlining these types, consider one of:

  1. Import and reference the SDK types via JSDoc: @property {import('onebusaway-sdk/resources/arrival-and-departure').ArrivalAndDepartureListResponse} [arrivalsAndDeparturesResponse]
  2. Or, if the SDK import paths don't resolve cleanly in JSDoc, create a single shared src/lib/types.js (or .d.ts) file that re-exports or aliases the SDK types, similar to the pattern already used in src/lib/serverCache.js with its @typedef declarations.

3. StopPane.svelte -- frequency: any | null still uses any

File: src/components/stops/StopPane.svelte, line 56

This defeats the purpose of the PR. The SDK types frequency as string | undefined. The correct JSDoc type is {string} [frequency] (optional string).


4. StopPane.svelte -- Props typedef is incomplete

File: src/components/stops/StopPane.svelte, lines 96-101

The Props typedef only declares stop and arrivalsAndDeparturesResponse, but the destructuring on lines 96-101 also extracts handleUpdateRouteMap and tripSelected. This means TypeScript/JSDoc checking would flag those as invalid properties. While this is a pre-existing issue, a PR specifically about correcting types should either fix it or at minimum note it as out-of-scope.

The types for those would be:

@property {((event: {detail: any}) => void) | null} [handleUpdateRouteMap]
@property {((event: {detail: any}) => void) | null} [tripSelected]

5. StopMarker.svelte -- stop type is missing fields the component actually uses

File: src/components/map/StopMarker.svelte, line 32

The component accesses r?.code on line 32 in the route name derivation:

.map((r) => r?.shortName || r?.code || (r?.id ? String(r.id).split('_').pop() : null))

But the route type in the PR only declares { id: string, shortName: string, type: number } -- it omits code. The SDK's References.Route type does not include code either, so this is also a pre-existing data concern, but the new type should at minimum not be more restrictive than what the code actually touches. Consider adding code?: string to the route sub-type.

Similarly, the stop type in the SDK includes code?: string, parent: string, routeIds: string[], and staticRouteIds: string[] -- the inline type only captures a subset.


6. Minor: Deleted blank lines

The diff removes blank lines between the typedef and the let declaration in Accordion.svelte (line 18-19) and StopMarker.svelte (line 23-24). These are cosmetic but should be consistent with the rest of the codebase. Most other components have a blank line separating the typedef from the destructuring.


Summary of requested changes

File Severity Issue
Accordion.svelte Bug {string} must be {string[]}
StopPane.svelte Architecture 70-line inline type should use SDK types or a shared typedef file
StopPane.svelte Bug Type starts at data level but prop holds full response (with code, data, etc.)
StopPane.svelte Incomplete frequency: any | null still uses any
StopPane.svelte Incomplete handleUpdateRouteMap and tripSelected missing from Props
StopMarker.svelte Incomplete Route sub-type missing code field that the component accesses

The TabLink.svelte change (any to string) is correct and looks good.

I'd recommend fixing the Accordion bug and reconsidering the approach for StopPane before merging. The direction of this PR is right -- eliminating any is important work -- but the replacement types need to be accurate and maintainable.

Copy link
Copy Markdown
Member

@aaronbrethorst aaronbrethorst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Bruce, glad to see work on tightening up the types in this codebase -- it's the kind of unglamorous work that pays dividends down the road. I found a few issues with the types as written that need to be addressed.

Critical Issues (2 found)

1. items in Accordion.svelte is typed as {string} but used as an array

The prop is initialized with $bindable([]), iterated with .includes(), spread with [...items, id], and passed to new Set(items). The type must be {string[]}, not {string}.

- * @property {string} [items] - Props to track all possible item IDs
+ * @property {string[]} [items] - Props to track all possible item IDs

Accordion.svelte:13

2. arrivalsAndDeparturesResponse type doesn't match actual usage in StopPane.svelte

The inline type starts at the data level (with entry and references as top-level keys), but the component accesses arrivalsAndDeparturesResponse.data.references.routes on line 165. The actual prop holds the full OBA response wrapper which includes code, currentTime, text, version, and data. As written, the type would cause type errors at every call site.

StopPane.svelte:35-92

Important Issues (3 found)

3. 70-line inline JSDoc duplicates (and diverges from) existing SDK types

The onebusaway-sdk package already ships TypeScript declarations for these response shapes in node_modules/onebusaway-sdk/resources/arrival-and-departure.d.ts and shared.d.ts. The hand-written types are already drifting -- for example, the SDK defines Situation.summary and Situation.description as { lang?: string, value?: string } objects, not plain strings.

Rather than hand-maintaining 70 lines of inline JSDoc, consider either:

  • Importing the SDK types via JSDoc (@type {import('onebusaway-sdk/resources/...').SomeType})
  • Or creating a shared src/lib/types.js with @typedef definitions that can be reused across components

This would also make the types more maintainable as the API evolves.

StopPane.svelte:35-92

4. frequency: any | null still uses any

The whole purpose of this PR is to replace any types, but one slipped through. The SDK types frequency as string | undefined.

StopPane.svelte:56

5. Missing code field in StopMarker route type

The component accesses r?.code on line 32 in the route name derivation, but the new type for routes omits the code property:

.map((r) => r?.shortName || r?.code || ...)

Add code?: string to the route sub-type.

StopMarker.svelte:7-17, 32

Suggestions (1 found)

  • The Props typedef in StopPane.svelte only declares stop and arrivalsAndDeparturesResponse, but the destructuring on lines 98-99 also pulls handleUpdateRouteMap and tripSelected. While pre-existing, if you're already touching this typedef it would be good to add those too. StopPane.svelte:96-101

Strengths

  • Correct and clean types for TabLink.svelte (href: string) and StopMarker.svelte (onClick: () => void, icon using the FontAwesome import type).
  • Good initiative tackling the any type cleanup across the codebase.

Recommended Action

  1. Fix the stringstring[] bug in Accordion.
  2. Fix the response wrapper mismatch in StopPane (the type needs a data wrapper, or the access pattern needs to change).
  3. Address the leftover any on frequency.
  4. Add the missing code field to StopMarker's route type.
  5. Consider whether the 70-line inline type is the right approach vs. importing from the SDK or extracting shared types.

Verdict: Request Changes

@Bruce-Mahagwa
Copy link
Copy Markdown
Author

Comments addressed in new commit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Replace any types with proper interfaces in component props

2 participants