Skip to content

fix: Improve types to remove unsafe casts#31

Merged
miles-grant-ibigroup merged 8 commits intofares-overhaulfrom
fares-overhaul-type-improvements
Jul 16, 2025
Merged

fix: Improve types to remove unsafe casts#31
miles-grant-ibigroup merged 8 commits intofares-overhaulfrom
fares-overhaul-type-improvements

Conversation

@daniel-heppner-ibigroup
Copy link

taking a crack at making a better fare product type for the right side of the calculation functions

return (
<>
<FaresV2Table legs={multipleFareOptionsItinerary.legs} />
{/* <FaresV2Table legs={multipleFareOptionsItinerary.legs} /> */}

Choose a reason for hiding this comment

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

there seems to be some bad data in this one, it doesn't conform to the GraphQL schema

Choose a reason for hiding this comment

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

This sounds like an OTP issue and not our issue

Copy link
Author

@daniel-heppner-ibigroup daniel-heppner-ibigroup Jul 8, 2025

Choose a reason for hiding this comment

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

did you grab this mock recently? OTP will never return malformed data. I'll double check to make sure I'm right about this being incorrect but maybe the schema changed slightly more recently to be more strict?

Choose a reason for hiding this comment

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

I double checked and I think I was wrong about how the GraphQL schema works. Since FareProduct has a couple different implementations, I thought it had to be one of those (and both of those have a required price field). But actually, I believe you can have a fare product that is just the "parent" object and doesn't use one of the two implementations, which would allow it to not have a price. Since my code currently crashes with the missing price, I have a bit more work to do here...

Copy link

@miles-grant-ibigroup miles-grant-ibigroup left a comment

Choose a reason for hiding this comment

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

I like your changes but disagree that we should implement them right away. I'd prefer to merge what we have now to avoid having to make fares v2 support a breaking change

return (
<>
<FaresV2Table legs={multipleFareOptionsItinerary.legs} />
{/* <FaresV2Table legs={multipleFareOptionsItinerary.legs} /> */}

Choose a reason for hiding this comment

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

This sounds like an OTP issue and not our issue

@daniel-heppner-ibigroup
Copy link
Author

okay, made some fixes, let me know what you think!

Copy link

@miles-grant-ibigroup miles-grant-ibigroup left a comment

Choose a reason for hiding this comment

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

Still some issues

Copy link

@miles-grant-ibigroup miles-grant-ibigroup left a comment

Choose a reason for hiding this comment

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

trip-details fare summary isn't working right (try with otp-rr if you can't replicate). Not sure what's going on there. otherwise everything looks great thanks

}
return clonedFare;
const alreadySeen = seenFareIds?.indexOf(fare.id) > -1;
const currency = fare.product.price.currency;

Choose a reason for hiding this comment

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

You could inline this

Suggested change
const currency = fare.product.price.currency;
const {currency} = fare.product.price;

alternateFareProducts: relevantFareProducts.splice(1).map(fp => fp.product),
price: cheapestRelevantFareProduct?.product?.price,
productUseId: cheapestRelevantFareProduct?.id,
appliedFareProduct: relevantFareProducts[0]?.product,

Choose a reason for hiding this comment

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

use cheapest please

if (!prev.some(p => p.productUseId === cur.productUseId)) {
prev.push({ productUseId: cur.productUseId, price: cur.price });
prev.push({
productUseId: cur.productUseId,

Choose a reason for hiding this comment

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

We could get this from the applied fare product?

Choose a reason for hiding this comment

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

actually no, that's a different ID. That's the fare product ID but the product use ID is the random string that belongs to that instance of the fare product.

Choose a reason for hiding this comment

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

I did refactor this to use an object and look up the keys in the reducer for performance.

index > 0 && <TransferIcon size={16} />}
{price
? renderFare(price?.currency?.code, price?.amount)
{legPrice

Choose a reason for hiding this comment

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

add a comment that legprice exists when original amount exists. There's probably a lot of cleanup possible overall but we can put it as a todo for now

Copy link

@miles-grant-ibigroup miles-grant-ibigroup left a comment

Choose a reason for hiding this comment

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

Beautiful and clean I am so excited

@miles-grant-ibigroup miles-grant-ibigroup merged commit a8c716f into fares-overhaul Jul 16, 2025
2 checks passed
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.

3 participants