fix/use correct type declaration for props with any types - phase 1#417
fix/use correct type declaration for props with any types - phase 1#417Bruce-Mahagwa wants to merge 2 commits intoOneBusAway:developfrom
Conversation
aaronbrethorst
left a comment
There was a problem hiding this comment.
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 inregisterItem) - 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.ts→ArrivalAndDepartureListResponseonebusaway-sdk/resources/stop.d.ts→StopRetrieveResponseonebusaway-sdk/resources/shared.d.ts→References,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
Situationtype hassummaryanddescriptionas 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,totalStopsInTripon arrivals;code,parent,staticRouteIds,wheelchairBoardingon 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:
- Import and reference the SDK types via JSDoc:
@property {import('onebusaway-sdk/resources/arrival-and-departure').ArrivalAndDepartureListResponse} [arrivalsAndDeparturesResponse] - 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 insrc/lib/serverCache.jswith its@typedefdeclarations.
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.
aaronbrethorst
left a comment
There was a problem hiding this comment.
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 IDsAccordion.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.jswith@typedefdefinitions 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.svelteonly declaresstopandarrivalsAndDeparturesResponse, but the destructuring on lines 98-99 also pullshandleUpdateRouteMapandtripSelected. 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) andStopMarker.svelte(onClick: () => void, icon using the FontAwesome import type). - Good initiative tackling the
anytype cleanup across the codebase.
Recommended Action
- Fix the
string→string[]bug in Accordion. - Fix the response wrapper mismatch in StopPane (the type needs a
datawrapper, or the access pattern needs to change). - Address the leftover
anyonfrequency. - Add the missing
codefield to StopMarker's route type. - Consider whether the 70-line inline type is the right approach vs. importing from the SDK or extracting shared types.
Verdict: Request Changes
|
Comments addressed in new commit |
Closes #404