Vehicle rental overlay graphql migration #1440
Conversation
…-graphql-migration
…-graphql-migration
…icle-rental-overlay-graphql-migration
…-graphql-migration
…l-overlay-graphql-migration
…-graphql-migration
…-graphql-migration
…tal-overlay-graphql-migration
…-graphql-migration
daniel-heppner-ibigroup
left a comment
daniel-heppner-ibigroup
left a comment
There was a problem hiding this comment.
Can you add some more types to the DefaultMap type before I approve?
lib/components/map/default-map.tsx
Outdated
| const micromobility = rentalVehicles.filter( | ||
| (vehicle) => | ||
| vehicle.vehicleType && | ||
| vehicle.vehicleType?.formFactor !== FormFactor.CAR |
There was a problem hiding this comment.
This will filter out any vehicles where vehicleType is undefined. Is that intentional? Do scooters/bikes never have undefined? Under what circumstance can it be undefined?
You should be able to remove the ? from the second part of the condition since the null check has already happened.
There was a problem hiding this comment.
Removed the redundant null check.
I think it's better to be more specific here and not include vehicles with an undefined vehicleType. It's better for a user to clearly not see their incorrectly-typed vehicles (and then fix them) than for users to incorrectly see vehicles that don't fit the expected type. Open to discussion though.
| setLocation: any | ||
| setViewedStop: any | ||
| viewedRouteStops: any | ||
| } |
There was a problem hiding this comment.
I think you could define a bunch of these. There are some low hanging fruit like intl -> IntlShape.
There was a problem hiding this comment.
I think I was able to get all of them (commit). Let me know if any stick out to you!
There was a problem hiding this comment.
Thank you!! Perfect
binh-dam-ibigroup
left a comment
binh-dam-ibigroup
left a comment
There was a problem hiding this comment.
Thanks for the changes. Seems to work and looks clean. Consider the suggested style tweaks and constants use before proceeding.
| export function findBikeRentalStations() { | ||
| return function (dispatch) { | ||
| dispatch( | ||
| createGraphQLQueryAction( | ||
| vehicleRentalStationsQuery, | ||
| {}, | ||
| bikeRentalResponse, | ||
| bikeRentalError, | ||
| { | ||
| rewritePayload: (payload) => | ||
| payload.data.vehicleRentalStations.filter( | ||
| vehicleRentalStationFilter('BICYCLE') | ||
| ) | ||
| } | ||
| ) | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| export const carRentalResponse = createAction('CAR_RENTAL_RESPONSE') | ||
| export const carRentalError = createAction('CAR_RENTAL_ERROR') | ||
|
|
||
| export function findCarRentalStations() { |
There was a problem hiding this comment.
Maybe for another PR: There is probably a way to make that query once and update the bike and rental states at the same time instead of filtering out the results. We could throttle the callbacks from the respective overlays too.
| { | ||
| rewritePayload: (payload) => | ||
| payload.data.vehicleRentalStations.filter( | ||
| vehicleRentalStationFilter('CAR') |
There was a problem hiding this comment.
Use constant FormFactor.CAR.
There was a problem hiding this comment.
Interestingly, I think my other usages of FormFactor.CAR are incorrect. Within the types package, FormFactor is a union type:
In the default map component, this means that we get type safety when using 'BICYCLE' to compare against station.vehicleType.formFactor, since formFactor carries the FormFactor type:
Unfortunately, this apiV2 file is not TypeScript, so we don't get that additional safety here.
Description:
This PR updates the
bikeRentalQueryto use graphQL instead of the deprecated REST endpoint.Note that the updates to OTP-UI found in this PR must be merged to correctly use the response that comes back from graphQL.PR Checklist: